On Thu, 7 Nov 2024 20:31:58 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Actually if we do `patternCache.putIfAbsent(pattern,
>> Pattern.compile(pattern.replace("*", ".*")))` we'll be computing Pattern on
>> every call regardless if it's already present in cache. Also `putIfAbsent`
>> makes no guarantees of atomicity. Better candidate here seems to be
>> `computeIfAbsent` method, but still it would not be as fast as the current
>> solution because it would add an extra `get` call to the cache interaction.
>> That's how exactly `computeIfAbsent` method works:
>>
>> ` * <pre> {@code
>> * if (map.get(key) == null) {
>> * V newValue = mappingFunction.apply(key);
>> * if (newValue != null)
>> * map.put(key, newValue);
>> * }
>> * }</pre>
>> `
>
> Good point, `computeIfAbsent` is what you would want. Both `putIfAbsent` and
> `computeIfAbsent` are done atomically though (the javadoc states that).
>
> I think it boils down to whether the existing code is ok and would not cause
> any unexpected behavior. AFAICT, there is a small chance for a race condition
> where more than one thread could end up computing and storing the same
> Pattern, but it should be the same algorithm, so there should be no negative
> side effects.
Oh, I was reading the javadoc for the base `Map` class, not for the
`ConcurrentHashMap`. Yes, those operations are atomic. Also, I kind of missed
the fact that `computeIfAbsent` returns the value, so it basically would be the
same functionality as the current implementation but more concise and atomic.
I'll make the change.
You are right that there would be no negative side effects anyhow, it's the
same pattern.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21841#discussion_r1833356025
