[8u] RFR: 8203190: SessionId.hashCode generates too many collisions

[Andrew John Hughes]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20190516/6b8dca2e/signature.asc>