RFR: JDK-8199216: Memory leak and quadratic layout time with nested nodes (hbox) and pseudo-class in style sheet [v2]

[John Hendrikx]

On Fri, 31 Mar 2023 15:32:06 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> John Hendrikx has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Simplify ImmutablePseudoClassSetsCache and avoid an unnecessary copy
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 584:
> 
>> 582:      * @param obj the object to cast, cannot be {@code null}
>> 583:      * @return a type T, or {@code null} if the argument was not of this type
>> 584:      * @throws NullPointerException when {@code obj} is {@code null}
> 
> Previously, this method always returned an instance of `T`. Now that is not the case, it might also simply return `null` if the argument passed into it is an instance of a different class. I think it makes sense to also return `null` when the argument passed into the method is `null`.

I'm a bit unsure why that would be an improvement. Passing `null` to a function that doesn't expect it should IMHO never just return `null` but should instead be considered a programming error and result in a stack trace.  Passing in a non-null value that can't be casted is explicitly documented now that it would result in `null`.  One is a caller error, the other isn't IMHO (as the caller can't check if it is castable without another method -- I considered adding an `instanceof` method).

Or maybe I'm reading too much in to this and you are just pointing out that the function has changed from its previous contract -- I think this is okay as `BitSet` is not public API, nor are any of its subclasses.

> modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 588:
> 
>> 586:     protected abstract T cast(Object obj);
>> 587: 
>> 588:     protected long[] getBits() {
> 
> Since your patch already contains some cleanup work: can you make this method `final`? The way it's specified at the moment looks like it was made to be overridable, which is clearly not useful.

Sure, I don't mind, the class is not public though, nor are its subclasses.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1154758868
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1154758778