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

Serialisers.findWriters primitive wrapper lookup performance #37656

Closed
franz1981 opened this issue Dec 11, 2023 · 4 comments · Fixed by #37655
Closed

Serialisers.findWriters primitive wrapper lookup performance #37656

franz1981 opened this issue Dec 11, 2023 · 4 comments · Fixed by #37655
Labels
area/rest kind/enhancement New feature or request
Milestone

Comments

@franz1981
Copy link
Contributor

franz1981 commented Dec 11, 2023

Describe the bug

#37646 da11e0f has by accident made quarkus to search for the writers in the hot path, making evident a possible performance improvement

image

Probably this is not supposed to be an hot path, but in case it will, it "could" be addressed at

where's

could be replaced by some different mechanism eg a chain of equality check guarded by Class::isPrimitive, which is an intrinsic.

@franz1981 franz1981 added the kind/bug Something isn't working label Dec 11, 2023
@geoand geoand added kind/enhancement New feature or request area/rest and removed kind/bug Something isn't working triage/needs-triage labels Dec 11, 2023
@geoand
Copy link
Contributor

geoand commented Dec 11, 2023

Would simply doing:

if (klass.isPrimitive() && primitivesToWrappers.containsKey(klass)) 

be any better? I assume it would

@geoand
Copy link
Contributor

geoand commented Dec 11, 2023

I am also wondering if findResourceWriters can be improved while maintaining the semantics...

@franz1981
Copy link
Contributor Author

franz1981 commented Dec 11, 2023

There are few factors in place here:

In short, in theory there are no reasons why it should be that slower, but still:

  • it requires accessing the `HashMap''s different references which hold the keys to find it
  • it requires pointer chasing the value's in the entry to be returned back

If the map is small enough, the map version should suffer from less branch-mispredictions (than the other version using the chain of if), but probably, in this case, due to the many indirections, is still slower.
The other pain point is that HashMap can can be reused in many occassions in the JIT code and if for "any" reasons (eg the stack depth is unlucky!) it couldn't be inlined, than it should relies on the type profile informations of the not inlined version of the same class, used elsewhere, with different key/value types, causing megamorphism or other performance quirk, on Object::hashCode or Object::equals.
In this case the safer choice, given that the cascade of if is not that long, the performance behaviour of the new version is just easier to predict and very expected, with none of the previous downsides.

@geoand
Copy link
Contributor

geoand commented Dec 11, 2023

Thanks for the analysis!

@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants