On Mon, 2 Mar 2026 16:31:24 GMT, Andy Goryachev <[email protected]> wrote:
> 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?
As always with these kinds of optimizations: it depends on the application.
Generally, you shouldn't expect this particular issue to be noticeable. But
when you have 1000 of these "not generally noticeable" small issues, quantity
can become a quality of its own.
> 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.
> 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.
> 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()`.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/2091#issuecomment-3986606443
PR Review Comment: https://git.openjdk.org/jfx/pull/2091#discussion_r2874359146
PR Review Comment: https://git.openjdk.org/jfx/pull/2091#discussion_r2874385130
PR Review Comment: https://git.openjdk.org/jfx/pull/2091#discussion_r2874396462
