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

Optimize ObjectNode findValue(s) and findParent(s) fast paths #4008

Merged
merged 1 commit into from Jul 4, 2023

Conversation

schlosna
Copy link
Contributor

@schlosna schlosna commented Jul 1, 2023

ObjectNode::findValue(String) and ObjectNode::findParent(String) currently traverse the node's children entries looking for a matching propertyName, leading to O(n) access time per level in an ObjectNode graph.

Both of these methods could be optimized to perform a O(1) LinkedHashMap.get(String) lookup for the fast path where given ObjectNode contains a child property with that propertyName, before falling back to the slow path traversing all child values.

Copy link
Member

@cowtowncoder cowtowncoder left a comment

Choose a reason for hiding this comment

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

LGTM

@cowtowncoder cowtowncoder merged commit 8050a1e into FasterXML:2.16 Jul 4, 2023
3 checks passed
@cowtowncoder cowtowncoder changed the title Optimize ObjectNode findValue(s) and findParent(s) fast paths Optimize ObjectNode findValue(s) and findParent(s) fast paths Jul 4, 2023
cowtowncoder added a commit that referenced this pull request Jul 5, 2023
@cowtowncoder
Copy link
Member

Thank you @schlosna ! Merged for inclusion in 2.16.0

@cowtowncoder cowtowncoder added this to the 2.16.0 milestone Jul 5, 2023
@JooHyukKim
Copy link
Member

This is super clean improvement 👍🏻 May I ask just one question tho, in what cases could traveral (the original way) still necessary?

@cowtowncoder
Copy link
Member

Lookups are recursive, i.e. find at any level, not just current one. For that it's still needed.
This also means that overall old code could actually be faster for some very special cases (very deep nesting with little to no branching).

@schlosna
Copy link
Contributor Author

schlosna commented Jul 5, 2023

Excellent, thanks @cowtowncoder !

@JooHyukKim
Copy link
Member

JooHyukKim commented Jul 5, 2023

Lookups are recursive, i.e. find at any level, not just current one. For that it's still needed.

Ahhh, I totally missed that. Thank you for the explanation! 🙏🏼

@schlosna schlosna deleted the ds/2.16/findValue branch July 14, 2023 22:12
JooHyukKim added a commit to JooHyukKim/jackson-databind that referenced this pull request Nov 28, 2023
}
foundSoFar.add(jsonNode);
return foundSoFar;
Copy link
Member

Choose a reason for hiding this comment

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

And here's the problem that was reported as #4229: we are to collect ALL matches, not just main-level one

}
foundSoFar.add(jsonNode.asText());
return foundSoFar;
Copy link
Member

Choose a reason for hiding this comment

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

... and here

cowtowncoder pushed a commit that referenced this pull request Nov 29, 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.

None yet

3 participants