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

Qute: reflection fallback value resolver optimization #43308

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Sep 16, 2024

  • add ValueResolver#getCachedResolver() so that reflection value resolver can optimize the cached resolver and save two concurrent hash map lookups and 2 MemberKey instances

I can observe ~ 20% higher throughput in the reflection value resolver benchmark (when compared to the main branch).

cc @franz1981

@mkouba mkouba requested a review from gastaldi September 16, 2024 07:17
@quarkus-bot quarkus-bot bot added the area/qute The template engine label Sep 16, 2024
@mkouba
Copy link
Contributor Author

mkouba commented Sep 16, 2024

RESULTS SUMMARY          |3.8.6 (Base)             |3.14.4                   |999-SNAPSHOT             
=========================|Score    |Error  |Diff   |Score    |Error  |Diff   |Score    |Error  |Diff   
-------------------------|-------------------------|-------------------------|-------------------------
HelloParser              |      694|      5|       |      696|     10|     0%|      717|     17|    +3%
HelloSimple              |    26663|    964|       |    26660|    357|     0%|    34158|    584|   +28%
IfComplex                |     6178|     74|       |     6068|    118|    -2%|     6572|    193|    +6%
IfSimple                 |     8377|    148|       |     8460|    121|    +1%|     8762|    115|    +5%
IncludeSimple            |    22377|    380|       |    22254|    159|    -1%|    26297|    518|   +18%
JavaBeanValueResolver    |    32473|   1188|       |    32840|    661|    +1%|    37745|   1242|   +16%
LetComplex               |     6412|    238|       |     6319|    134|    -1%|     7029|    200|   +10%
LetSimple                |    17488|    153|       |    17480|    247|     0%|    21307|    375|   +22%
Loop15                   |    35226|    387|       |    35212|    715|     0%|    45507|    509|   +29%
Loop50                   |    11052|    136|       |    10890|    372|    -1%|    14142|    313|   +28%
NameResolver             |    19547|    223|       |    19359|    273|    -1%|    22759|    673|   +16%
Reflect                  |     8296|    297|       |     8416|     66|    +1%|    11741|    234|   +42%
When                     |     9418|    234|       |     9566|    246|    +2%|     9900|    199|    +5%

@mkouba
Copy link
Contributor Author

mkouba commented Sep 16, 2024

Hmm, if we decide to backport this one then there are few more optimizations (#42892, #42982, #43003 and #43006) that need to be backported as well...

@geoand
Copy link
Contributor

geoand commented Sep 16, 2024

Okay, I'll remove the label and let you folks decide :)

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 16, 2024
@franz1981
Copy link
Contributor

did you tried to run it with -t 4?
@mkouba I believe this will be more fun (and with even better results!) :P
BTW well done!

This comment has been minimized.

@mkouba
Copy link
Contributor Author

mkouba commented Sep 16, 2024

The failure looks related... investigating.

- add ValueResolver#getCachedResolver() so that reflection value
resolver can optimize the cached resolver and save two concurrent hash
map lookups and 2 MemberKey instances
@mkouba mkouba force-pushed the qute-reflect-res-cache branch from 4449c78 to 6941566 Compare September 16, 2024 14:40
@mkouba
Copy link
Contributor Author

mkouba commented Sep 16, 2024

did you tried to run it with -t 4? @mkouba I believe this will be more fun (and with even better results!) :P BTW well done!

I fixed the problem related to the CI failure and also improved the benchmark but the results look very similar.

RESULTS SUMMARY          |3.8.6 (Base)             |3.14.4                   |999-SNAPSHOT             
=========================|Score    |Error  |Diff   |Score    |Error  |Diff   |Score    |Error  |Diff   
-------------------------|-------------------------|-------------------------|-------------------------
Reflect                  |     3990|    206|       |     4068|    180|    +2%|     5792|     52|   +45%

and for -t 4 (old CPU Intel(R) Core(TM) i5-6260U CPU @ 1.80GHz, 2 cores):

RESULTS SUMMARY          |3.8.6 (Base)             |3.14.4                   |999-SNAPSHOT             
=========================|Score    |Error  |Diff   |Score    |Error  |Diff   |Score    |Error  |Diff   
-------------------------|-------------------------|-------------------------|-------------------------
Reflect                  |     7796|    383|       |     7888|    115|    +1%|    11360|    350|   +46%

And no big difference with quick and dirty test on my laptop (12th Gen Intel(R) Core(TM) i7-1270P, 12 cores).

Copy link

quarkus-bot bot commented Sep 16, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 6941566.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 integration-tests/rest-client-reactive

io.quarkus.it.rest.client.BasicTest.shouldCreateClientSpans - History

  • expected: <1> but was: <2> - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: expected: <1> but was: <2>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:531)
	at io.quarkus.it.rest.client.BasicTest.shouldCreateClientSpans(BasicTest.java:216)

@franz1981
Copy link
Contributor

In my case, probably due to the very high clock, the original lookup was a major scalability killer, but maybe it depends by a mix of number of cores and high frequency (my machine is stable clocked at 4.5 GHz on 32 cores)

@mkouba
Copy link
Contributor Author

mkouba commented Sep 17, 2024

my machine is stable clocked at 4.5 GHz on 32 cores

Oops, that's quite a difference ;-)

@mkouba mkouba merged commit 75a5fe3 into quarkusio:main Sep 17, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 17, 2024
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants