Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance & Code Style, avoid passing iterators around #340

Closed
TheMatthew opened this issue May 22, 2017 · 4 comments
Closed

Performance & Code Style, avoid passing iterators around #340

TheMatthew opened this issue May 22, 2017 · 4 comments

Comments

@TheMatthew
Copy link

Hi,
I have an issue with the JSONObjecy#keys() method. It states it is an iterator, though the name implies it is an access to a collection. I have also noticed that is it used often in a while(iter.hasNext){foo = iter.next; ...) pattern. This is not as optimal as using an enhanced for loop on a JSONObject#keySet(). It is murky as to whether or not I am allowed to contribute code yet, but if you replace all #keys accesses by #keyset for loops you can go from parsing a 2 GB file in 125 seconds to 115. this is not an order of magnitude like @johnjaylward 's #337 patch, but it's still pretty decent.

One could argue the reason for doing the iterator is to avoid issues with concurrent modifications. I would argue that a readwritelock would be a safer approach in general.

Thanks for making such a great product by the way.

@johnjaylward
Copy link
Contributor

The history of those loops is from pre-java5 when the enhanced for loop was not available. I think they just haven't been updated due to them working and no complaints from performance. I don't think it should be an issue to update these.

@stleary
Copy link
Owner

stleary commented May 22, 2017

Everyone is welcome to submit pull requests. Since the project is in maintenance mode, there are limits to what kind of changes are accepted. Every pull request carries a risk of introducing regressions, or negatively impacting existing users. Please see the FAQ for more information: https://github.com/stleary/JSON-java/wiki/FAQ.

@johnjaylward
Copy link
Contributor

@stleary, I believe he was implying he wanted permission from his company before spending company time on it.

I opened a PR for the refactor. Let me know if it meets your needs. It's currently based off of Master and not my other PR that is scheduled for merge.

@stleary
Copy link
Owner

stleary commented May 23, 2017

This issue is addressed in pull request #341. Please post any future comments on that thread.

@stleary stleary closed this as completed May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants