CssMetaData.combine()

[John Hendrikx]

Hi Andy,

Let me start to say that I had no problem with this PR being merged as I 
already agreed with one of the first versions.

Sometimes then on the same PR there can be some discussions on what else 
can be done in this area, potentially maybe even alleviating the need 
for the change (X/Y problem, ie. why do you need this method? Because 
you need to concatenate lists, but the underlying reason is that the CSS 
property initialization is somewhat clumsy).

On 01/12/2023 01:11, Andy Goryachev wrote:
>
> Dear colleagues:
>
> there were a couple of comments after I 
> withdrewhttps://github.com/openjdk/jfx/pull/1296 
> <https://github.com/openjdk/jfx/pull/1296>for reasons of frustration, 
> so I wanted to respond to those in the openjfx list.
>
> >I pondered that back when I was working on replacing these static 
> initializers with the|.of|collection variants. It doesn't work here 
> for problem stated above - we need to modify an unmodifiable list, 
> which is why I didn't touch them in that pass. While the proposed 
> method is useful for eliminating some ugly syntax, cementing a 
> questionable API with more a public API doesn't seem to me like the 
> right direction. If the method is made internal only, then that's 
> fine. Alternatively, if the method is made useful outside of this 
> specific context, then even if it won't be used here, it could be used 
> in other places, and that's also fine.
>
> Even though the syntax is ugly, the current implementation of the 
> static getClassCssMetaData() is/nearly perfect/, considering the lack 
> of some kind of a 'lazy' keyword in java.
>
It may be "nearly perfect" from an optimization viewpoint, but it is 
clumsy and unwieldy for anyone wanting to implement CSS properties.
>
> What the current code does is two things - a lazy initialization, 
> meaning the code will get executed only when needed, and it has zero 
> per-instance overhead.  I don't think anyone can suggest a better way 
> of doing it.
>
This was already mentioned on the PR, but I'll repeat it here: what is 
the lazy initialization for?  As soon as these Nodes need to be shown, 
all the metadata will have been queried already. I don't see any benefit 
making them lazy so you can create Nodes faster, as long as they are 
never shown.

> I don't buy Nir's argument about "questionable API".  The API is 
> codified by Node.getCssMetaData() and the current implementation will 
> be perfect with the proposed utility method (and maybe we can address 
> some other comments 
> fromhttps://git.openjdk.org/jfx/pull/1293#discussion_r1411406802 
> <https://git.openjdk.org/jfx/pull/1293#discussion_r1411406802>).
>
How can there be any doubt that this API is questionable?  It ignores a 
core feature of Java (inheritance) and moves this burden to the user by 
calling static methods of its direct parent... in order to implement CSS 
property **inheritance** -- it also burdens any subclass with the 
caching of these properties (because "performance"), and to make those 
properties publicly (and statically) available so another subclass might 
"inherit" them.

The API is clumsy enough that I loathe creating stylable properties for 
the sheer amount of boilerplate that surrounds them.

Some alternatives have been suggested, but are shot down without 
thinking along to see if there might be something better possible here.  
Solutions where some of the common logic is moved to either Node or the 
CSS subsystem are certainly worth considering.

> ... a few bytes and cpu cycles would get saved ...
>
This is not for you specifically, but JavaFX has a lot of 
"optimizations", some resulting in really questionable patterns that 
have/are hurting us:

- Reimplementing core collection classes for some benefit, but then only 
partially implementing them (and often buggy), and/or completely 
breaking the collection contract [BitSet]

- Lazy initialization in many places that IMHO is not needed (benchmark 
should be time to show window, anything accessed before that need not be 
lazy, and is likely counterproductive)

- Using plain arrays in many places, with a lot of custom code that's 
already available in some standard collection class or as a standard 
pattern; the custom code often has untested edge cases that contain bugs 
[ExpressionHelper]

- Making things mutable; surely mutating something must always be faster 
than having to create a new object? Except that if there's a lot of 
duplication going on because these objects are unshareable (because 
mutable), the cost/benefit is no longer so clear (but try to prove that 
with a micro benchmark) [PseudoClassState / StyleClassSet]

- Also see many usages of LinkedList, a class that if you'd never use 
it, you'd be better off 99.999% of the time; use of that class should 
always be explained in a comment, and proven to be better with a 
benchmark [too many places to list]

The list goes on; many of the optimizations I've seen would make sense 
for C/C++, but not for Java.  Now I don't mind some optimizations, but 
practically none of them are documented (deviating from the standard 
path always deserves an explanation for the next developer) and I 
suspect they were never verified either.  I've done extensive 
"optimization" before, with benchmarks, and you'd be surprised what is 
actually faster, and what makes no difference whatsoever -- even then, 
after benchmarking, if the difference is small, it's best to use 
established patterns, as that's what the JDK optimizes for.  What is 
marginally faster now, may not be faster on the next JDK, with a 
different GC, when run in a real application (caches will be used 
differently), or on a completely different architecture.

--John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20231201/ae50cdc1/attachment.htm>