On Sun, 24 Nov 2024 08:55:20 GMT, Laurent Bourgès <[email protected]> wrote:
>> Here is a constant declaration (hotspot friendly): >> https://github.com/bourgesl/jdk-official/blob/d0b770c938be6b5b3a0176148265ef33184a9b8b/src/java.desktop/share/classes/sun/java2d/marlin/Renderer.java#L38 >> >> private static final int ALL_BUT_LSB = 0xFFFFFFFE; > > '500 iterations' looks high, maybe 100? > It is a recursion ? You could try max recursion depth + 3 samples (first, > middle, last) ? > > Or this magic threshold could be tunable ac runtime using special static > getter using a System property + default value, used by constant declaration > (final static is mandatory): > > https://github.com/bourgesl/jdk-official/blob/d0b770c938be6b5b3a0176148265ef33184a9b8b/src/java.desktop/share/classes/sun/java2d/marlin/MarlinProperties.java#L50 > > https://github.com/bourgesl/jdk-official/blob/d0b770c938be6b5b3a0176148265ef33184a9b8b/src/java.desktop/share/classes/sun/java2d/marlin/MarlinProperties.java#L303 I added a constant `MAX_UNBALANCED_TOP_NODES = 100`, and I moved some comments from outside the method to inside the method. Regarding recursion: I'm reluctant to explore the tree recursively because I want `needsRebalance` to be a light/fast call. (It will be called with every addition.) The primary use case I'm worried about is code that resembles this example in Container.java: public synchronized void addContainerListener(ContainerListener l) { if (l == null) { return; } containerListener = AWTEventMulticaster.add(containerListener, l); newEventsOnly = true; } There are several of these static `AWTEventMulticaster.add(X, X)` methods. This is (IMO) the recommended/primary way to interact with the AWTEventMulticaster. This implementation (without rebalancing) created a tree where the *top* of the tree was guaranteed to be unbalanced. (Specifically: every left node may be a tree, and every right node will be a leaf.) It's not necessary to recursively scan the entire tree to identify this condition, so I'd prefer not to make `needsRebalance` more complex than it needs to be to identify this usage. (That is: there's no need to look at the middle or bottom of the tree, or even to identify the size or height of the tree.) Regarding a System property: I'm also reluctant to add this; it feels like bloat to me that nobody will ever use. Do you think you (or anyone else) will want to customize this property over time? My broad goal here is to prevent a StackOverflowError; not to fine-tune tree performance. I definitely appreciate seeing `System.getProperty("x")` in several important pieces of code, but IMO altering the `MAX_UNBALANCED_TOP_NODES` field will only ever yield negligible changes. (Or that could be the subject of another ticket, with another test case?) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21962#discussion_r1874643208
