On 16/05/2019 18:51, Severin Gehwolf wrote: > Hi, > > Could I please get a review of this OpenJDK 8u only fix? JDKs 11+ don't > seems to have this issue as with the TLS 1.3 feature (JDK-8196584) > SessionId.hashCode() got changed to use Arrays.hashCode() already. > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/01/webrev/ > Bug: https://bugs.openjdk.java.net/browse/JDK-8203190 > > The rationale for the fix are these assumptions: > > a) elements in trees on hash collision of LinkedHashMap used internally > by the MemoryCache class become prohibitively large for many SessionId > entries in the cache, b) moderate speed of the new hashCode() impl will > not have a detrimental effect on performance overall. > > Comparison of performance of hashCode impls[1]: > > Benchmark Mode Cnt Score Error Units > SessionIDBench.newHashCode thrpt 100 43649538.284 ± 678702.696 ops/s > SessionIDBench.oldHashCode thrpt 100 94068843.923 ± 1379930.266 ops/s > > Collision testing[2] showed that indeed, the current hashCode() > implementation of SessionId produces more collissions and, thus, > produce more elements in trees for collision resolution in the > underlying LinkedHashMap. The default cache expiry is 24 hours per > entry and this can result in millions of entries in the cache in some > circumstances[3]. > > Before: > ################################################## > Collision test for 100 sessions: > ------------------------------------------------ > Total number of collisions: 4 > Max length of collision list over all buckets: 2 > > Collision test for 20480 sessions: > ------------------------------------------------ > Total number of collisions: 18311 > Max length of collision list over all buckets: 30 > > Collision test for 10000000 sessions: > ------------------------------------------------ > Total number of collisions: 9996395 > Max length of collision list over all buckets: 9709 > ################################################## > > After: > ################################################## > Collision test for 100 sessions: > ------------------------------------------------ > Total number of collisions: 0 > > Collision test for 20480 sessions: > ------------------------------------------------ > Total number of collisions: 0 > > Collision test for 10000000 sessions: > ------------------------------------------------ > Total number of collisions: 11530 > Max length of collision list over all buckets: 2 > ################################################## > > > Testing: Above testing, and make test. No new failures. > > Thoughts? > > Thanks, > Severin > > [1] > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/SessionIDBench.java > [2] > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/SessionIdCollissionTest.java > [3] https://bugs.openjdk.java.net/browse/JDK-8210985 >
Change looks good. Is there a reason the tests aren't included in the webrev? I think it would be better to have them checked in, even if they can't be run automatically. They will need copyright headers and I'd correct the spelling of 'collision' too :-) -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222 https://keybase.io/gnu_andrew
signature.asc
Description: OpenPGP digital signature
