On Mon, 2 Mar 2026 20:02:38 GMT, Andy Goryachev <[email protected]> wrote:
>> 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? 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 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? I want to make these changes to improve the code. I don't really see the problem here...? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2091#discussion_r2874459382 PR Review Comment: https://git.openjdk.org/jfx/pull/2091#discussion_r2874473929
