Skip to content

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

Merged
cowtowncoder merged 1 commit into
FasterXML:2.16from
schlosna:ds/2.16/findValue
Jul 4, 2023
Merged

Optimize ObjectNode findValue(s) and findParent(s) fast paths#4008
cowtowncoder merged 1 commit into
FasterXML:2.16from
schlosna:ds/2.16/findValue

Conversation

@schlosna

@schlosna schlosna commented Jul 1, 2023

Copy link
Copy Markdown
Contributor

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.

@cowtowncoder cowtowncoder left a comment

Copy link
Copy Markdown
Member

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
@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

schlosna commented Jul 5, 2023

Copy link
Copy Markdown
Contributor Author

Excellent, thanks @cowtowncoder !

@JooHyukKim

JooHyukKim commented Jul 5, 2023

Copy link
Copy Markdown
Member

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 = new ArrayList<>();
}
foundSoFar.add(jsonNode);
return foundSoFar;

Copy link
Copy Markdown
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 = new ArrayList<>();
}
foundSoFar.add(jsonNode.asText());
return foundSoFar;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

... and here

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