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