RFR: 8342782: AWTEventMulticaster throws StackOverflowError using AquaButtonUI [v2]

[Jeremy]

On Thu, 7 Nov 2024 21:21:57 GMT, Phil Race <prr at openjdk.org> wrote:

>> Jeremy has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - 8342782: Removing confusing javadoc
>>    
>>    This is in response to this PR comment:
>>    https://github.com/openjdk/jdk/pull/21962#discussion_r1833401410
>>  - 8342782: adding curly brackets
>>    
>>    This is in response to these PR comments:
>>    https://github.com/openjdk/jdk/pull/21962#discussion_r1833399928
>>    https://github.com/openjdk/jdk/pull/21962#discussion_r1833413838
>>  - 8342782: removing new comment in javadoc
>>    
>>    This is in response to this PR comment:
>>    https://github.com/openjdk/jdk/pull/21962#pullrequestreview-2422181991
>
> src/java.desktop/share/classes/java/awt/AWTEventMulticaster.java line 960:
> 
>> 958:      * Occasionally (after approximately 500 invocations) the root
>> 959:      * AWTEventMulticaster is rebalanced.
>> 960:      *
> 
> I am not sure this comment is needed. If you think it is needed then it would need a CSR, and the comment should be tagged @implNote

I removed that comment.

> src/java.desktop/share/classes/java/awt/AWTEventMulticaster.java line 985:
> 
>> 983:      * <p>
>> 984:      * The first time this method is invoked it will convert a tree that has a degree
>> 985:      * of approximately 500 to a tree with a degree of approximately 10.
> 
> where does it do that ? I just see it deciding if there's a need to rebalance.

I removed this sentence. It confusingly conflated this method with the method that actually did the rebalancing.

> src/java.desktop/share/classes/java/awt/AWTEventMulticaster.java line 991:
> 
>> 989:         while (true) {
>> 990:             if (++level > 500)
>> 991:                 return true;
> 
> our coding standards require that you always include the body in { .. }

@prrace : I added { .. }

@bourgesl : I'm struggling to define this as a constant in a helpful way. Is there an example of a similar constant somewhere in the codebase you could refer me to for comparison?

I think part of my challenge here is the AWTEventMulticaster doesn't promise to be any particular kind of tree. (For example: if it promised it was a self-balancing tree, then we'd know what our obligations were to meet that expectation.)

This PR's implementation is intended to be a fast heuristic/educated guess, but it does not scan the entire height of the left or right node. This makes it ... hard for me to name.

(Relatedly: the value 500 is arbitrary, and I'm happy to change it if someone would rather it be closer to 50 or 1000.)

> src/java.desktop/share/classes/java/awt/AWTEventMulticaster.java line 1018:
> 
>> 1016:      */
>> 1017:     private static EventListener rebalance(EventListener[] array, int index0, int index1) {
>> 1018:         if (index0 == index1)
> 
> { .. } again

I added { .. }

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21962#discussion_r1855390611
PR Review Comment: https://git.openjdk.org/jdk/pull/21962#discussion_r1855390855
PR Review Comment: https://git.openjdk.org/jdk/pull/21962#discussion_r1855390839
PR Review Comment: https://git.openjdk.org/jdk/pull/21962#discussion_r1855390868