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

Disable Jackson field name interning #2583

Merged
merged 2 commits into from Apr 4, 2023

Conversation

schlosna
Copy link
Contributor

@schlosna schlosna commented Apr 4, 2023

Before this PR

Seeing synchronization contention in com.fasterxml.jackson.core.util.InternCache.intern(String) via JFR with services processing high volumes of SMILE encoded data, where we are reusing ObjectMapper I don't believe we are failing to close the JsonParser. See FasterXML/jackson-core#946

image

After this PR

==COMMIT_MSG==
Interning introduces excessive contention
==COMMIT_MSG==

Possible downsides?

This potentially increases allocations and GC work necessary for deserialization.

Long term we are probably better off pushing String deduplication to the Garbage Collector via -XX:+UseStringDeduplication per palantir/sls-packaging#1378

Relevant context:

https://shipilev.net/jvm/anatomy-quarks/10-string-intern/

G1 and Shenandoah GC support string deduplication as of JDK 17

Only enable on JDK 17+ due to fix https://bugs.openjdk.org/browse/JDK-8277981

JDK 18+ enable string deduplication for SerialGC, ParallalGC, and ZGC.
https://malloc.se/blog/zgc-jdk18

Interning introduces excessive contention
FasterXML/jackson-core#946
@changelog-app
Copy link

changelog-app bot commented Apr 4, 2023

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Interning introduces excessive contention

Check the box to generate changelog(s)

  • Generate changelog entry

@carterkozak
Copy link
Contributor

My understanding, which I haven't validated, is that interning is applied atop canonicalization (so that writing/reading N objects will still reuse field names based on the same property as mapped from a field via data-binding) but not arbitrary
equal string value property names across different types.
If this is the case, avoiding the intern step sounds ideal as it would not necessarily have much negative impact, but would substantially reduce cross-thread contention.

@carterkozak
Copy link
Contributor

My previous comment on this PR was off base, this doesn't have much to do with smile or id-based lookups that smile employs.

Given the small size of the cache, I don't think we're any better off with the cache than without it, because many of our services define more parameter names than the intern-cache max-size. Where the intern cache may be providing us some benefit is through its recency bias, however I imagine that disappears when multiple operations execute in parallel anyhow.

I suspect that it's an upstream bug that map keys are considered for interning, but regardless of whether that is resolved, I think we're going to be better off opting out of the feature given the cache size. The strings will be very short-lived in most cases (used to map to a method) and avoiding the cache may help hotspot escape analysis.

@schlosna schlosna marked this pull request as ready for review April 4, 2023 18:47
Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

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

Thank you for finding this, @schlosna !

@bulldozer-bot bulldozer-bot bot merged commit 9a1d9e1 into develop Apr 4, 2023
@bulldozer-bot bulldozer-bot bot deleted the ds/disable-jackson-intern branch April 4, 2023 19:31
@svc-autorelease
Copy link
Collaborator

Released 7.46.0

@schlosna
Copy link
Contributor Author

schlosna commented Apr 4, 2023

Thanks for the in depth test @carterkozak !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants