8378895: Reduce object allocations in Renderer.getPeerInstance() by mstr2 · Pull Request #2091 · openjdk/jfx

mstr2 · 2026-03-01T03:41:47Z

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.

/reviewers 2

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8378895: Reduce object allocations in Renderer.getPeerInstance() (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2091/head:pull/2091
$ git checkout pull/2091

Update a local copy of the PR:
$ git checkout pull/2091
$ git pull https://git.openjdk.org/jfx.git pull/2091/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2091

View PR using the GUI difftool:
$ git pr show -t 2091

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2091.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-03-01T03:43:01Z

👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-03-01T03:43:59Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2026-03-01T03:44:23Z

@mstr2
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

mlbridge · 2026-03-01T03:48:06Z

Webrevs

nlisker · 2026-03-01T13:34:34Z

modules/javafx.graphics/src/main/java/com/sun/scenario/effect/impl/Renderer.java

-        new HashMap<>(1);
-    private Map<String, EffectPeer> peerCache =
-        Collections.synchronizedMap(new HashMap<String, EffectPeer>(5));
+    private static final Map<FilterContext, Renderer> rendererMap = new HashMap<>(1);


If this is meant to initialize the map to a size of 1 then HashMap.newHashMap should be used.

nlisker · 2026-03-01T13:36:32Z

modules/javafx.graphics/src/main/java/com/sun/scenario/effect/impl/Renderer.java

+        }
+    }
+
+    private final Map<PeerCacheKey, EffectPeer> peerCache = Collections.synchronizedMap(new HashMap<>(5));


Isn't a ConcurrentHashMap a better alternative?

No, I don't think so, and I think Collections.synchronizedMap is also pointless here. If this is supposed to allow concurrent access to the peerCache map, the original implementation is defective: Collections.synchronizedMap clearly states that iteration must be synchronized on the map instance, which this code fails to do.

I've changed the implementation to use a normal HashMap, and synchronize manually where it is accessed or modified. For this purpose, I've removed the Renderer.getPeers() method, which is only called in PPSRenderer.dispose() to iterate over the peers and dispose each one of them. Instead, the operation is now encapsulated in Renderer.clearPeers().

andy-goryachev-oracle

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?

andy-goryachev-oracle · 2026-03-02T15:28:51Z

modules/javafx.graphics/src/main/java/com/sun/scenario/effect/impl/Renderer.java

+
+        @Override
+        public boolean equals(Object o) {
+            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?

andy-goryachev-oracle · 2026-03-02T15:34:12Z

modules/javafx.graphics/src/main/java/com/sun/scenario/effect/impl/Renderer.java

+    public final EffectPeer getPeerInstance(FilterContext fctx, String name, int unrollCount) {
+        synchronized (peerCache) {
+            // The lookup key is only used to look up mappings, it is never inserted into the map.
+            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?

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.

andy-goryachev-oracle · 2026-03-02T16:27:11Z

modules/javafx.graphics/src/main/java/com/sun/scenario/effect/impl/Renderer.java

+    protected final void clearPeers() {
+        EffectPeer<?>[] peers;
+
+        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?

I want to make these changes to improve the code. I don't really see the problem here...?

mstr2 · 2026-03-02T20:00:40Z

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.

Allocation-free map lookup

d26b8d2

openjdk bot added the rfr Ready for review label Mar 1, 2026

add test

c14ac68

nlisker reviewed Mar 1, 2026

View reviewed changes

mstr2 added 2 commits March 1, 2026 17:25

don't use Collections.synchronizedMap

ed6b72f

HashMap.newHashMap

9a059bb

andy-goryachev-oracle reviewed Mar 2, 2026

View reviewed changes

review comment

d48d08b

Conversation

mstr2 commented Mar 1, 2026 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Mar 1, 2026

Uh oh!

openjdk bot commented Mar 1, 2026

Uh oh!

openjdk bot commented Mar 1, 2026

Uh oh!

mlbridge bot commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstr2 Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstr2 Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstr2 commented Mar 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

mstr2 commented Mar 1, 2026 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 1, 2026 •

edited

Loading

mstr2 Mar 1, 2026 •

edited

Loading

mstr2 Mar 2, 2026 •

edited

Loading

andy-goryachev-oracle Mar 2, 2026 •

edited

Loading