On Sun, 1 Mar 2026 16:30:13 GMT, Michael Strauß <[email protected]> wrote:
>> The current implementation of `Renderer.getPeerInstance()` looks up mappings
>> by concatenating strings to form a combined key:
>>
>> peer = peerCache.get(name + "_" + unrollCount);
>>
>>
>> This can be improved by not using strings to store the combined key, but
>> using a simple `PeerCacheKey` class instead.
>> The `Renderer` class then uses a single reusable `PeerCacheKey` object to
>> look up mappings, making the lookup allocation-free.
>
> Michael Strauß has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - HashMap.newHashMap
> - don't use Collections.synchronizedMap
I see several structural changes here (synchronization, changing the order of
events around DISPOSED) that go beyond a simple optimization.
I am all for replacing string concatenation with a compound key like object, my
main question though is how big of the problem is this? Do we really incur
significant overhead creating those keys?
modules/javafx.graphics/src/main/java/com/sun/scenario/effect/impl/Renderer.java
line 99:
> 97: @Override
> 98: public boolean equals(Object o) {
> 99: return o instanceof PeerCacheKey other
typically
if(o == this) {
return true;
}
modules/javafx.graphics/src/main/java/com/sun/scenario/effect/impl/Renderer.java
line 266:
> 264: synchronized (peerCache) {
> 265: // The lookup key is only used to look up mappings, it is
> never inserted into the map.
> 266: peerCacheKey.name = name;
this code is not re-entrant. can you guarantee it's safe?
modules/javafx.graphics/src/main/java/com/sun/scenario/effect/impl/Renderer.java
line 308:
> 306: EffectPeer<?>[] peers;
> 307:
> 308: synchronized (peerCache) {
Having two monitors involved (peerCache here and peer's one elsewhere) might
lead to a deadlock. Just by looking at the code, I see one potential with
createPeer() in Renderer:273 . Could you take a look please?
-------------
PR Review: https://git.openjdk.org/jfx/pull/2091#pullrequestreview-3876923151
PR Review Comment: https://git.openjdk.org/jfx/pull/2091#discussion_r2873079356
PR Review Comment: https://git.openjdk.org/jfx/pull/2091#discussion_r2873108512
PR Review Comment: https://git.openjdk.org/jfx/pull/2091#discussion_r2873382653
