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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Peter Levart-3
On Sun, 20 Dec 2020 22:41:33 GMT, PROgrm_JARvis <[hidden email]> 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?
>>
>> 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.
>
>> 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?
>
> Fair enough; the solution proposed by Claes in #1855 seems to be a better alternative.
>
> Currently, I will keep this PR open but I expect this to be superseded by the latter.

Hi Claes,
Would flattening the state of MD5 bring any further improvements?
https://github.com/plevart/jdk/commit/92bf48ff58f0ce9648e49466dbf1befebbf49083

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

PR: https://git.openjdk.java.net/jdk/pull/1821
Reply | Threaded
Open this post in threaded view
|

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

PROgrm_JARvis
On Thu, 7 Jan 2021 17:09:14 GMT, Claes Redestad <[hidden email]> wrote:

>> Hi Claes,
>> Would flattening the state of MD5 bring any further improvements?
>> https://github.com/plevart/jdk/commit/92bf48ff58f0ce9648e49466dbf1befebbf49083
>
>> Hi Claes,
>> Would flattening the state of MD5 bring any further improvements?
>> [plevart@92bf48f](https://github.com/plevart/jdk/commit/92bf48ff58f0ce9648e49466dbf1befebbf49083)
>
> I think it might, marginally, but it seemed to me that the implCompress0 MD5 intrinsic is using `state` so I've not tried that yet since rewriting and checking the intrinsic code is a lot of work. (I've blogged about my experiments in this area and mentioned this issue in passing: https://cl4es.github.io/2021/01/04/Investigating-MD5-Overheads.html#accidental-constraints)

>
>
> @JarvisCraft This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

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

PR: https://git.openjdk.java.net/jdk/pull/1821
Reply | Threaded
Open this post in threaded view
|

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

Claes Redestad-2
On Wed, 10 Feb 2021 14:08:22 GMT, PROgrm_JARvis <[hidden email]> wrote:

>>> Hi Claes,
>>> Would flattening the state of MD5 bring any further improvements?
>>> [plevart@92bf48f](https://github.com/plevart/jdk/commit/92bf48ff58f0ce9648e49466dbf1befebbf49083)
>>
>> I think it might, marginally, but it seemed to me that the implCompress0 MD5 intrinsic is using `state` so I've not tried that yet since rewriting and checking the intrinsic code is a lot of work. (I've blogged about my experiments in this area and mentioned this issue in passing: https://cl4es.github.io/2021/01/04/Investigating-MD5-Overheads.html#accidental-constraints)
>
>>
>>
>> @JarvisCraft This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

FWIW, I ended up doing two related improvements:

- #1855 - which reduced overheads of constructing MD5, SHA1, SHA2, SHA5 `MessageDigest` objects, and slightly improving digest performance
- #1933 - reducing cost of `MessageDigest.getInstance` in general by better internal caching of `Constructor` objects, among other things. With this there's now no extra allocations when looking up something that has been looked up before.

Together these enhancements bring the overheads of `UUID.nameUUIDFromBytes` down close to what you can get from cloning from a `ThreadLocal` `MD5` object as suggested here. Taking the ambient overheads of `ThreadLocal`s into account I'd say it's not worth adding this cache, and would suggest withdrawing this PR and closing the RFE.

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

PR: https://git.openjdk.java.net/jdk/pull/1821
Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman-2
In reply to this post by PROgrm_JARvis
On Wed, 10 Feb 2021 14:08:22 GMT, PROgrm_JARvis <[hidden email]> wrote:

>>> Hi Claes,
>>> Would flattening the state of MD5 bring any further improvements?
>>> [plevart@92bf48f](https://github.com/plevart/jdk/commit/92bf48ff58f0ce9648e49466dbf1befebbf49083)
>>
>> I think it might, marginally, but it seemed to me that the implCompress0 MD5 intrinsic is using `state` so I've not tried that yet since rewriting and checking the intrinsic code is a lot of work. (I've blogged about my experiments in this area and mentioned this issue in passing: https://cl4es.github.io/2021/01/04/Investigating-MD5-Overheads.html#accidental-constraints)
>
>>
>>
>> @JarvisCraft This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@JarvisCraft Can you re-run your benchmarks in light of the changes that @cl4es pointed out in the previous comment? We think this PR can be closed.

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

PR: https://git.openjdk.java.net/jdk/pull/1821
Reply | Threaded
Open this post in threaded view
|

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

PROgrm_JARvis
In reply to this post by PROgrm_JARvis
On Wed, 10 Feb 2021 14:08:22 GMT, PROgrm_JARvis <[hidden email]> wrote:

>>> Hi Claes,
>>> Would flattening the state of MD5 bring any further improvements?
>>> [plevart@92bf48f](https://github.com/plevart/jdk/commit/92bf48ff58f0ce9648e49466dbf1befebbf49083)
>>
>> I think it might, marginally, but it seemed to me that the implCompress0 MD5 intrinsic is using `state` so I've not tried that yet since rewriting and checking the intrinsic code is a lot of work. (I've blogged about my experiments in this area and mentioned this issue in passing: https://cl4es.github.io/2021/01/04/Investigating-MD5-Overheads.html#accidental-constraints)
>
>>
>>
>> @JarvisCraft This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

> @JarvisCraft Can you re-run your benchmarks in light of the changes that @cl4es pointed out in the previous comment? We think this PR can be closed.

My apologies for the late reply,

I'll try re-running these with the merged branch ASAP (the delay is due to me changing the workstation thus having some job to reconfigure the environment).

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

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