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

TypeFactory cache performance degradation with constructSpecializedType() #3876

Closed
carterkozak opened this issue Apr 12, 2023 · 2 comments
Closed
Milestone

Comments

@carterkozak
Copy link
Contributor

carterkozak commented Apr 12, 2023

Describe the bug
TypeFactory cache interactions taking seconds in real-world scenarios under heavy load (e.g. Jersey ProviderBase, where 'constructSpecializedType' is used) where the operation itself takes microseconds.

TypeFactory.constructSpecializedType adds elements to the cache which contain PlaceholderForType type arguments. These type arguments all have the same hashCode (based on Object.class plus a hard-coded value for all placeholders) while overriding equals to perform reference equality. Each time that constructSpecializedType is called, more data is added to the cache with the same hash code, but equality never succeeds due to instances differing, not only reducing cache hit probability, but making it increasingly difficult to find the values we want due to following pointers on heavy cache collisions where the data structure degenerates into a linked list.
I've committed a change which bypasses the cache in cases where PlaceholderForType is used. I don't know if this is an ideal solution, but it improves the case where I've encountered this edge case.

Version information
Impacts versions prior to 2.14.0, but the issue becomes more pronounced in 2.14.0 due to #3531 which doesn't simply clear the map upon reaching max size. #3675 seems to make the issue even more pronounced in some environments due to heavy hash collisions.

To Reproduce
Reproducer can be found here: FasterXML/jackson-benchmarks#5

PR here: #3875

@carterkozak carterkozak added the to-evaluate Issue that has been received but not yet evaluated label Apr 12, 2023
@cowtowncoder cowtowncoder added 2.15 and removed to-evaluate Issue that has been received but not yet evaluated labels Apr 12, 2023
@cowtowncoder
Copy link
Member

I regret merging of #3531; we have made a few highly speculative optimization attempts that may have been net negative (in the worst case). :-(

But as to solution, yes, my first instinct is that the change makes sense.

I'll see if we can get the fix merged tomorrow since I agree that this sounds like a gnarly issue.

And if we can prove this via 2.15.0-rc3, I am open to limited backport into 2.14(.3) if we are happy with safety aspects.
That may actually need to wait until 2.15.0 release if we want to get wider feedback (true .0 release tends to bring out more bug reports than all RCs combined).

carterkozak added a commit to palantir/conjure-java-runtime that referenced this issue Apr 12, 2023
FasterXML/jackson-databind#3876
FasterXML/jackson-benchmarks#5

The linked benchmarks show a small advantage of caching over
not caching in most cases, in the best case for a cache: when
a single operation is repetedly executed. In practice, the cache
is used for all operations and sees heavy evictions, where contention
is more likely and has greater impact on users.

Given the hash collision issues that we're aware of, and the impacts
we've observed, we will remove the cache until a version is available
which isn't impacted by the linked issue for performance that's
more predictable.
cowtowncoder pushed a commit that referenced this issue Apr 13, 2023
…ecializedType` (#3875)

Improve TypeFactory cache performance with `constructSpecializedType` (...)
@cowtowncoder cowtowncoder changed the title TypeFactory cache performance degradation with constructSpecializedType TypeFactory cache performance degradation with constructSpecializedType() Apr 13, 2023
@cowtowncoder cowtowncoder added this to the 2.15.0-rc3 milestone Apr 13, 2023
cowtowncoder added a commit that referenced this issue Apr 13, 2023
@carterkozak
Copy link
Contributor Author

carterkozak commented Jun 14, 2023

Closing this as resolved by #3875

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

No branches or pull requests

2 participants