RFR: 8258588: MD5 MessageDigest in java.util.UUID should be cached

[Claes Redestad]

On Sun, 20 Dec 2020 19:48:43 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> I have to say that introducing a ThreadLocal here seems like a step in the wrong direction. With a ThreadLocal, if I read this correctly, a MessageDigest will be cached with each thread that ever calls this API, and it won't be garbage collected until the thread dies. Some threads live indefinitely, e.g., the ones in the fork-join common pool. Is this what we want? Is UUID creation so frequent that each thread needs its own, or could thread safety be handled using a simple locking protocol?
>> 
>> I'd like to see a demonstration that fetching and allocating an MD5 MessageDigest instance on each call is indeed a performance problem (either because it's too slow, or because it takes too much memory). Based on that we might or might not want to do some optimization of some form or another. Or we might want to optimize creation in MessageDigest instead of caching instances in UUID. More fundamentally, I'd like to see an answer to the question, "What problem is this change intended to solve?"
>
>> I have to say that introducing a ThreadLocal here seems like a step in the wrong direction. With a ThreadLocal, if I read this correctly, a MessageDigest will be cached with each thread that ever calls this API, and it won't be garbage collected until the thread dies. Some threads live indefinitely, e.g., the ones in the fork-join common pool. Is this what we want? Is UUID creation so frequent that each thread needs its own, or could thread safety be handled using a simple locking protocol?
> 
> This is a good point. Significant effort has gone into recent releases to reduce the use of TLs in the (JDK-8245309, JDK-8245308, JDK-8245304, JDK-8245302) so adding new usages is a disappointing. So I think this PR does need good justifications, and probably a lot more exploration of options.

As Stuart argues a TL approach is likely to look better in a microbenchmark, but make things worse in other regards.

I started exploring options to this in #1855 (not logged an RFE, yet), and I think there is potential to get most of the gains seen here without the introduction of a `ThreadLocal`. The current patch also speeds up MD5.digest for small chunks (+19% for 16 bytes, but less relative impact on larger chunks, down to being lost in the noise on 16Kb). Speed-up of `UUID.nameUUIDFromBytes` is somewhat modest right now (+4%, -17% allocations), but I think it can be improved further without complicating things too much. The MD5 intrinsic added by JDK-8250902 adds some constraints on the implementation that holds back some polish. This can be fixed, but requires some coordination.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1821