On Sun, 20 Dec 2020 19:48:43 GMT, Alan Bateman <al...@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

Reply via email to