On Mon, 2 Mar 2026 20:11:59 GMT, Michael Strauß <[email protected]> wrote:
>> 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?
>
> The reusable `peerCacheKey` instance is only used in these three lines:
>
> peerCacheKey.name = name;
> peerCacheKey.unrollCount = unrollCount;
> EffectPeer<?> peer = peerCache.get(peerCacheKey);
>
>
> Since the `Map.get()` method won't call out to other code, I don't think
> there is any chance that something can go wrong in this particular segment of
> code.
>
>> You've also added synchronization on a different object here, which might
>> create a deadlock.
>
> I don't see any potential problem that wasn't there before. Previously, this
> method synchronized on `this`, now it synchronizes on `peerCache`. That's not
> a material difference in itself, considering that there's only one other
> place where we synchronize on `peerCache`, and I've made sure that it won't
> call out to other methods.
I'm also confused why this is synchronized. Looking at the subclasses, I can
find patterns like that:
synchronized (this) {
state = NOTREADY;
}
and comments like `// Must be called on the renderer thread`.
Nothing else is synchronized or uses synchronized collections. So if most of
the code in the `Renderer`s is called with multiple threads, there will be many
races and incoinsistent data. So might be worth to check in a follow-up.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2091#discussion_r3078916081
