Repository navigation
[PERF] Jackson reflection-free serialization/deserialization - #42946
Conversation
|
@mabartos, what is happening in Matrix 1? Win/Lose alternates between runs. |
We can't ensure the runs are 1:1 mapped (in a sense they have the same env conditions, etc.), f.e. there might be the case when some run for the New loses, but when compared with other run, it'd win. You can see that for the matrix 1, these values oscillates for the Old in range 1224-1492, where for the New 1385-1634. I haven't checked stdev and other props.
The default ones - ~5? OpenJDK 21.0.3 @pruivo It'd be nice to compare it in the KC benchmark that could show more interesting data. I can maybe also provide a comparison of flame graphs. |
|
And in term of native version, it is faster? |
@franz1981 We don't use native builds in Keycloak. Or are you referring to sth else? |
|
I remember an issue where keycloak got bad XP with native image but I wasn't aware is not available as native app 🙏 |
hmm, I'm guessing I'm not expecting the value to have such a large range between the best and worst runs.
I can run it locally. Do authentication/token refreshes go through Jackson serialization frequently enough to be measured by KC Benchmark? Or should I create a micro-benchmark (JMH)? |
|
Just FYI, loolking how reflection work, it got bad and worse as the number and type of fields grows and the number of serialized/deserialized types too. Not to mention the RSS consumed due to method handle generation, which won't happen in reflection free case |
@pruivo IMO, in order to find out what exactly is happening there, we'd need to go into
AFAIK, not really very frequent. I don't know what are exact current available scenarios in KC benchmark, so I cannot tell what would be the best. The point is, and would be nice-to-have, to run some benchmark with a ~real load and see how it improved the execution. |
|
You can use jbang with wrk2 and have fixed rate with coordinated omission free measurements AND compare latencies. The suggestion clearly is to make sure your application is the bottleneck which means running it with perf stat or pidstat and make sure is CPU (usr) intensive |
|
@mabartos I run the benchmark locally, so take the results with a grain of salt as I was using the PC while benchmarking. I run twice, and your PR seems more stable. The committed memory at the end of the test is lower with your PR. This PRMain (commit 29bee21) |
|
@pruivo Great! I think we have enough data to proceed with the approach. Thanks! @keycloak/cloud-native Could anyone look at this? Thanks! |
|
Did you measured the startup time? |
|
I would be +1 to use it, at the same time Quarkus is stating this is a technology preview feature. So I'd say we would need to wrap it as an opt-in preview feature in KC as we don't want to support it downstream, and some tests enabled and disabled. We might open a ticket with the Quarkus team make it non-preview eventually. Reading the docs, the following part worries me a bit:
|
|
@brunobat / @franz1981 - thank for joining this discussion. Do you have additional information for us on the maturity of the feature and any plans for graduation of this feature? |
|
I believe @franz1981 has details about why it's still in preview. |
@brunobat Nope. Should we expect some bigger differences?
@ahus1 Oh, totally missed this. Thanks for pointing this out! Yes, unfortunately, we'd need to have it disabled by default :( |
The expectation is that it will start faster because more work was done at build, however, I think we should measure the real world behavior differences in the C2 compiler under load, during warmup. |
|
Correct |
|
Yes, we were waiting for user feedback. We had some and we fixed some of the issues along the way. I haven't seen a new issue for a while now (but then it's hard to know how widely this feature is used). But we had feedback so it has been tested by some users. The issues were mostly about corner cases not handled properly in serialization/deserialization, not about structural unstability. Maybe you could provide the option with a default value to false for a while and then switch it to true at some point (and keeping the ability to disable it?). |
And if you do that, make sure you have a prominent note in the release notes to gather as much feedback as possible. |
Yes, that's the plan. Thank you all for your feedback! |
|
🙏🏽 |
|
@ahus1 Thanks for the feedback. Should be done now. Token exchange docsOldNew |
Closes keycloak#42945 Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Co-authored-by: Alexander Schwartz <alexander.schwartz@gmx.net> Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Co-authored-by: Václav Muzikář <vaclav@muzikari.cz> Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
|
@mabartos Some tests failed but seems unrelated? |
|
@vmuzikar Yes, those tests should be unrelated. Retried |
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
|
Resolved conflict |
|
Progress: We run the whole old testsuite (thousands of tests) with this settings for ~3 weeks and AFAIK, we have not encounter any issues - I think it's a nice indicator it works properly. |
|
That's great to hear! |
I've followed up on the guide provided by the Quarkus team. The Jackson-free serdes was mentioned during the presentation about Observability performance in Quarkus, so they're including that even in their benchmarks.
In the guide, there's mentioned ~12% improvement in the throughput for HTTP calls. I made a brief performance analysis, and these are my findings:
Performance analysis
wrkfrom the HyperfoilTest characteristic
host:8080/admin/realms/master/clientsendpoint was called (multiple clients that are deserialized) with the access token-- Matrix 1: 3 threads, 3 connections
-- Matrix 2: 10 threads, 10 connections
-- Matrix 3: 15 threads, 15 connections
Matrix 1 (3 threads, 3 connections)
Requests per second
Average: 5.196% improvement ✅
Matrix 2 (10 threads, 10 connections)
Requests per second
Average: 4.96% improvement ✅
Matrix 3 (15 threads, 15 connections)
Requests per second
Average: 6.052% improvement ✅
Augmentation time
As some work is moved to the build phase, we experience a small increase in the augmentation time (which should be ok):
Average Old: 6152ms
Average New: 6488ms
Difference: +336ms ❌
Difference in %: +6.20% ❌
Node characteristics
Conclusion
In total, in this brief performance analysis, we experience 5.40% ✅ improvement in the throughput in our API. Note that values for some runs differ as it might be influenced by other tasks done on the CPU and not related to this benchmark. This test only gets clients from the realm, so for some endpoints, where the serdes is not used much does not have a big effect. On the other hand, the effect should be higher when serdes is used frequently.
I think it's a nice improvement, and there should not be any issues I'm not aware of.
cc: @keycloak/cloud-native @keycloak/sre
cc: @franz1981 @brunobat