ArrayClassResolver #878
Open
ArrayClassResolver #878
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lifeinwild
commented
Jan 26, 2022
lifeinwild
commented
- in terms of functionality, ArrayClassResolver is completely equivalent to DefaultClassResolver.
- 60% faster in module only comparison.
- 17% faster in a case where new module is good.
- even or a bit faster in existing benchmarks.
- fully tested by existing all test cases.
This modification provides a faster lookup from classID to Registration.
A fix because an error occured after checkout latest remote version. And by changing Sample to String in the value of Map, the performance difference of ClassResolver is more clear in the score.
|
ArrayClassResolver vs DefaultClassResolver in existing benchmarks and a new benchmark deserializeCollection(). the benchmark of deserializeCollection() means the benchmark where new module is good as mentioned above. I said 17%, but when I did it over again, it was 22%. ArrayClassResolver makes sense only when idToRegistration is used. Some benchmarks of ArrayClassResolverBenchmark may not use ArrayClassResolver at all. ArrayClassResolver is generally superior to DefaultClassResolver, and may replace the DefaultClassResolver's idToRegistration implementation, If you agree on this benchmark result. |
|
@lifeinwild: Thanks for the PR! Your solution is interesting, but I don't think it's generic enough to be included as an additional class resolver in Kryo. Users have to be aware of how to use the new class resolver, e.g. high IDs would lead to very large array sizes. Changes to registrations rebuild the entire array. The current map-based approach doesn't have these issues. It would probably make more sense to cache registrations in |
|
I confirmed a very funny behavior about DefaultClassResolver. When kryo deserialize a Map, the order is a key, a value, a key, a value over and over. By the way, the existing cache system of memoizedClass remembers the last deserialized class. so, when it deserialize a value, memoizedClass is the class of key. when it deserialize a key, memoizedClass is the class of value. So the cache system is not working at all. Postscript: The existing cache system of memoizedClass is very efficient in some cases, but in deserializing Map, memoizedClassId and memoizedClassIdValue of DefaultClassResolver.java line 144 readClass() don't make sense. The ArrayClassResolver javadoc provides some notes on using it. In most cases, class IDs are sequential number, and I thought there is no problem. |
|
There are a few cases where DefaultClassResolver beats ArrayClassResolver. I imagine that it is when the cache system works effectively. If I implement memoizedClass cache system in ArrayClassResolver, ArrayClassResolver will win in all cases. |
|
I compared again with the memoizedClass version about all the cases ArrayClassResolver lost. It reversed in almost all but one case. There may be a new case of losing. I should calculate that automatically. |
To recommend Pooling for a specific risk of ArrayClassResolver. EsotericSoftware#878
|
I did more benchmarking. In most cases, ArrayClassResolver wins, but sometimes loses due to measurement errors. Another design idea is to support Map by two memoizedClasses in DefaultClassResolver. Pooling solves the problem of ArrayClassResolver. |
|
Now the risk of reconstructing array is same of standard classes such as HashMap. And the performance is balanced. |
|
The cache code of DefaultClassResolver don't work in ser/der of Map that has different classes for its key and value. You don't understand it maybe. It is important because huge objects are mainly collection. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.