On Mon, 2 Mar 2026 19:51:18 GMT, Michael Strauß <[email protected]> wrote:
>> 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;
>> }
>
> Yes, but since the lookup key is never inserted into the map, `o` is never
> `this`. That would be an optimization that'll never be hit.
makes sense. maybe add a comment then?
>> 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?
>
> To be fair, I don't know why the code here is synchronized at all. I only
> ever see this method be called on the renderer thread (as it should be).
> However, I don't think that I've changed anything substantial here: the
> method was synchronized before, and is synchronized now.
Hm, I meant something else: you are changing a peerCacheKey instance, are you
sure it will never re-enter the same method (and clobber the same instance?)
You've also added synchronization on a different object here, which might
create a deadlock. And you do bring another good point - if this code is
always called from the same thread, why synchronize at all?
>> 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?
>
> I'm not calling any dangerous method while holding a lock here. The code
> first copies the values of the map into a local array, then releases the
> lock, and only then calls `EffectPeer.dispose()`.
Right, my original comment should have been placed at L237, where inside a
block synchronized on `peerCache`, you are calling `createPeer()`. It looks
like the latter is not synchronized on anything (can't see past reflection
though), but if inside of `createPeer()` there is another code synchronized
around a different object, there `might be` deadlock.
It looks like there isn't, but my question remains: why do you want to
introduce these non-trivial changes in the first place, how big of a problem is
this allocation is?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2091#discussion_r2874397013
PR Review Comment: https://git.openjdk.org/jfx/pull/2091#discussion_r2874419056
PR Review Comment: https://git.openjdk.org/jfx/pull/2091#discussion_r2874456683
