On Sun, 16 Mar 2025 21:29:31 GMT, John Hendrikx <[email protected]> wrote:
>> modules/javafx.graphics/src/main/java/javafx/css/Styleable.java line 101:
>>
>>> 99: * only for its direct children!
>>> 100: *
>>> 101: * @return an immutable list of CSS meta data, never {@code null}
>>
>> Since we don't specify that `getCssMetaData()` should never return null, we
>> should consider also not specifying it here. Alternatively, maybe we should
>> add the non-null requirement to the existing `getCssMetaData()`.
>
> That might be a good idea, as there are several places in `CssStyleHelper`
> where `getCssMetaData` is assumed to be non-null already (and also a few
> locations where it is assumed it can be `null`). In other words, if the
> function would return `null`, FX would break currently.
>
> For example, `recalculateRelativeSizeProperties` will do:
>
> final List<CssMetaData<? extends Styleable, ?>> styleables =
> node.getCssMetaData();
> final int numStyleables = styleables.size();
>
> Which would be an NPE.
I think then you can do this as part of this PR, since you’re already touching
the `Styleable` interface.
>> modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line
>> 465:
>>
>>> 463: }
>>> 464:
>>> 465: private record StylingContext(Node node, CalculatedValue font,
>>> StyleMap styleMap, Set<PseudoClass> pseudoClasses) {}
>>
>> It might not be a good trade-off to create lots of transient objects on a
>> hot path just to save a few arguments in the calling convention. This would
>> be a nice improvement once we have value types in Java.
>
> I checked this before hand, there is quite a bit more going on in creating
> the cache keys (see `getTransitionStates`), and I think this extra object
> will therefore be lost in the noise.
I still think that it doesn’t carry its weight (bundling up private method
arguments in the same translation unit is not a big win). It will increase
churn on the GC, even if slightly, and many of these decisions taken together
may cause it to run more often or earlier.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1714#discussion_r1998018887
PR Review Comment: https://git.openjdk.org/jfx/pull/1714#discussion_r1998016829
