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

Capacity improvements for internal structures #348

Merged
merged 3 commits into from
Jun 12, 2017

Conversation

johnjaylward
Copy link
Contributor

@johnjaylward johnjaylward commented Jun 8, 2017

Key Changes:

  • Updates array constructor and bulk operations to best guess capacity information
  • Update JSONObject to allow best guess for initial capacity for internal operations

What problem does this code solve?
For some operations for JSONArray and JSONObject we already know the end-capacity of our internal structures, however, we grow them dynamically instead. This change reduces dynamic growth operations by providing capacity information ahead of time for certain operations.

Risks
low. If anything, this should be improvements for both CPU and memory for the cases effected by the change.

Changes to the API?
One new protected constructor added to JSONObject to allow specification of the internal hashmap capacity. I didn't feel this was valid as a public constructor as it exposes implementation details of the underlying structure.

Will this require a new release?
No, can be rolled into the next release

Should the documentation be updated?
No

Does it break the unit tests?
No test cases failed on the current master.

Was any code refactored in this commit?
Yes

Review status
ACCEPTED Starting 3 day window

…y information

* Update JSONObject to allow best guess for initial capacity.
JSONArray.java Outdated
@@ -154,7 +154,7 @@ public JSONArray(String source) throws JSONException {
* A Collection.
*/
public JSONArray(Collection<?> collection) {
this.myArrayList = new ArrayList<Object>();
this.myArrayList = new ArrayList<Object>(collection == null ? 0 : collection.size());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use an if/else statement here instead of 0 as initial capacity

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was that if you pass null, you are probably trying to create an empty JSONArray. But for backwards compatibility it may be better to use the standard capacity. Personally if I'm using this constructor it's because I've already finalized my object somewhere else and I'm just preparing it for serialization. But that generalization may not be what we want to imply.

I'll go ahead and change it.

@stleary
Copy link
Owner

stleary commented Jun 9, 2017

Looks like a reasonable enhancement, though most users will not likely notice a difference, Don't see a need for additional tests in this case. Starting 3 day window.

@stleary stleary merged commit 1add124 into stleary:master Jun 12, 2017
@johnjaylward johnjaylward deleted the ArrayPerformance branch July 7, 2017 16:35
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

Successfully merging this pull request may close these issues.

3 participants