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

JsonArray optimizations #4995

Closed
wants to merge 14 commits into from

Conversation

magicprinc
Copy link
Contributor

  • Better stream() and spliterator() implementations
  • ctor with initialCapacity
  • better copy ctor

1) use JDK Map.entry(key,value) instead of self-made Entry class
One caveat: Map.entry disallows null keys and values (Attempts to create Map.Entry using a null key or value result in NullPointerException)
2) (Linked)HashMap has DEFAULT_LOAD_FACTOR = 0.75f: if you create it with initialCapacity=X and put X items into it then HashMap resizes (with new internal array allocation and re-hash). One has to create HashMap with initialCapacity=X/0.75 e.g. for 10 keys initialCapacity must be at least 14
3) better inheritance: equals `if (getClass() != o.getClass()) return false;` rejects subclasses. if (!(o instanceof JsonObject)) return false allows subclasses and checks for null
1) skip implicit null check
2) MAP_CREATOR
1) use MAP_CREATOR
2) LinkedHashMap → HashMap (can be changed in MAP_CREATOR)
3) better initial capacity
better spliterator() and stream() implementations
Use Spliterator flags and map.size() to make them more performant
@vietj
Copy link
Member

vietj commented Dec 4, 2023

can you provide a single commit PR ?

if you need 10 commits for a PR then just do the 10 commits in your branch (that is your concern, not ours) and then do a PR once you believe the PR is ready with a single commit.

/**
* Create an instance from a List. The List is not copied.
*
* @param list the underlying backing list
*/
public JsonArray(List list) {
if (list == null) {
@SuppressWarnings({"rawtypes", "unchecked"})
Copy link
Member

Choose a reason for hiding this comment

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

that is an avoidable breaking change, we need to keep the List constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it really break?

I just thought widening is not a problem 🤷‍♀️

@vietj
Copy link
Member

vietj commented Dec 4, 2023

why this PR is also applying what your other PR about json object contains ?

we are not going to review the same code twice.

@vietj
Copy link
Member

vietj commented Dec 4, 2023

in other words this PR seems to be focusing on JsonArray, it should not care about JsonObject unless there is a relationship

@magicprinc
Copy link
Contributor Author

magicprinc commented Dec 4, 2023

@vietj I have made branch from feature/jsonObject just in haste 😥
The right way to do was/is: JsonObject PR first, JsonArray later

I will redo this PR as a single commit with JsonArray only.

@magicprinc
Copy link
Contributor Author

I close this PR, sorry, no time for rebase and force-push - deadline at job.
This PR has no much value. So:
#5002

@magicprinc magicprinc closed this Dec 4, 2023
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.

2 participants