RFR: 8308588: Unnecessary synchronized on GTKStyle#ICONS_MAP can be removed [v2]

Abhishek Kumar abhiscxk at openjdk.org
Fri Aug 30 05:01:19 UTC 2024


On Thu, 29 Aug 2024 12:42:49 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>>> > One slight concern is that, does GTKStockIcon has any role to play with synchronized block?
>>> 
>>> Sorry, I didn't understand what do you mean.
>> 
>> I meant, since `ICONS_MAP` stores `GTKStockIcon`  as value, I was wondering does that make any difference or need for the map to be synchronized? I also came across Immutable Maps which are inherently thread safe. If ICONS_MAP are immutable one, then Synchronized block is definitely redundant.
>
>> > > One slight concern is that, does GTKStockIcon has any role to play with synchronized block?
>> > 
>> > 
>> > Sorry, I didn't understand what do you mean.
>> 
>> I meant, since `ICONS_MAP` stores `GTKStockIcon` as value, I was wondering does that make any difference or need for the map to be synchronized?
> 
> In fact, `synchronized` block around `ICONS_MAP.get` had no effect.
> 
> To be correct, the value for `ICONS_MAP` had to be assigned inside a `synchronized` block too. Otherwise, it is not thread-safe.
> 
>> I also came across Immutable Maps which are inherently thread safe. If ICONS_MAP are immutable one, then Synchronized block is definitely redundant.
> 
> Effectively, `ICONS_MAP` is an immutable map. It's initialised in a static initialiser and is never modified afterwards.
> 
> As far as I can tell, `GTKStockIcon` objects stored in the map aren't modified either.
> 
> Thus, the entire structure is immutable and is thread-safe without additional synchronisation.

@aivanov-jdk I guess `Maps.of` can't be used here because there are few elements put inside a map after `tk.checkGtkVersion(3, 10, 0)` condition evaluation at [L1193](https://github.com/kumarabhi006/jdk/blob/73f7a5f15dbba54a98f3916ff1190520ac07874d/src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java#L1193). Instead used `Collections.unmodifiableMap` to make the `ICONS_MAP` read only.

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

PR Comment: https://git.openjdk.org/jdk/pull/20741#issuecomment-2320053428


More information about the client-libs-dev mailing list