<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi Andy,<br>
</p>
<p>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.</p>
<p>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).<br>
</p>
<div class="moz-cite-prefix">On 01/12/2023 01:11, Andy Goryachev
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:DM5PR1001MB2172D0DA0D52E4ECE5A37C67E581A@DM5PR1001MB2172.namprd10.prod.outlook.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style>@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}@font-face
{font-family:"Yu Gothic";
panose-1:2 11 4 0 0 0 0 0 0 0;}@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}@font-face
{font-family:"Iosevka Fixed SS16";
panose-1:2 0 5 9 3 0 0 0 0 4;}@font-face
{font-family:"Times New Roman \(Body CS\)";
panose-1:2 11 6 4 2 2 2 2 2 4;}@font-face
{font-family:"\@Yu Gothic";
panose-1:2 11 4 0 0 0 0 0 0 0;}p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
mso-ligatures:standardcontextual;}a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}code
{mso-style-priority:99;
font-family:"Courier New";}span.EmailStyle17
{mso-style-type:personal-compose;
font-family:"Iosevka Fixed SS16";
color:windowtext;}span.apple-converted-space
{mso-style-name:apple-converted-space;}.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}div.WordSection1
{page:WordSection1;}</style>
<div class="WordSection1">
<p class="MsoNormal"><span style="font-family:"Iosevka
Fixed SS16";color:#212121">Dear colleagues:</span><span
style="color:#212121"><o:p></o:p></span></p>
<p class="MsoNormal" style="caret-color: rgb(33, 33,
33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px">
<span style="font-family:"Iosevka Fixed
SS16";color:#212121"> </span><span
style="color:#212121"><o:p></o:p></span></p>
<p class="MsoNormal" style="caret-color: rgb(33, 33,
33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px">
<span style="font-family:"Iosevka Fixed
SS16";color:#212121">there were a couple of comments
after I withdrew<span class="apple-converted-space"> </span><a
href="https://github.com/openjdk/jfx/pull/1296"
title="https://github.com/openjdk/jfx/pull/1296"
moz-do-not-send="true"><span
style="font-family:"Calibri",sans-serif;color:#0078D7">https://github.com/openjdk/jfx/pull/1296</span></a>for
reasons of frustration, so I wanted to respond to those in
the openjfx list.</span><span style="color:#212121"><o:p></o:p></span></p>
<p class="MsoNormal" style="caret-color: rgb(33, 33,
33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px">
<span style="font-family:"Iosevka Fixed
SS16";color:#212121"> </span><span
style="color:#212121"><o:p></o:p></span></p>
<p class="MsoNormal" style="margin-left:.5in;caret-color:
rgb(33, 33, 33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px">
<span style="font-family:"Iosevka Fixed
SS16";color:#212121">><span
class="apple-converted-space"> </span></span><span
style="font-size:12.0pt;color:#212121">I pondered that back
when I was working on replacing these static initializers
with the<span class="apple-converted-space"> </span></span><code><span
style="font-size:12.0pt;color:#212121">.of</span></code><span
class="apple-converted-space"><span
style="font-size:12.0pt;color:#212121"> </span></span><span
style="font-size:12.0pt;color:#212121">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.</span><span
style="color:#212121"><o:p></o:p></span></p>
<p class="MsoNormal" style="caret-color: rgb(33, 33,
33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px">
<span style="font-size:12.0pt;color:#212121"> </span><span
style="color:#212121"><o:p></o:p></span></p>
<p class="MsoNormal" style="caret-color: rgb(33, 33,
33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px">
<span style="font-family:"Iosevka Fixed
SS16";color:#212121">Even though the syntax is ugly,
the current implementation of the static
getClassCssMetaData() is<span class="apple-converted-space"> </span><i>nearly
perfect</i>, considering the lack of some kind of a 'lazy'
keyword in java.</span><span style="color:#212121"><o:p></o:p></span></p>
</div>
</blockquote>
It may be "nearly perfect" from an optimization viewpoint, but it is
clumsy and unwieldy for anyone wanting to implement CSS properties.<br>
<blockquote type="cite"
cite="mid:DM5PR1001MB2172D0DA0D52E4ECE5A37C67E581A@DM5PR1001MB2172.namprd10.prod.outlook.com">
<div class="WordSection1">
<p class="MsoNormal" style="caret-color: rgb(33, 33,
33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px">
<span style="font-family:"Iosevka Fixed
SS16";color:#212121"> </span><span
style="color:#212121"><o:p></o:p></span></p>
<p class="MsoNormal" style="caret-color: rgb(33, 33,
33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px">
<span style="font-family:"Iosevka Fixed
SS16";color:#212121">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.</span><span style="color:#212121"><o:p></o:p></span></p>
</div>
</blockquote>
<p>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.<br>
</p>
<blockquote type="cite"
cite="mid:DM5PR1001MB2172D0DA0D52E4ECE5A37C67E581A@DM5PR1001MB2172.namprd10.prod.outlook.com">
<div class="WordSection1">
<p class="MsoNormal" style="caret-color: rgb(33, 33,
33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px">
<span style="font-family:"Iosevka Fixed
SS16";color:#212121"> </span><span
style="color:#212121"><o:p></o:p></span></p>
<p class="MsoNormal" style="caret-color: rgb(33, 33,
33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px">
<span style="font-family:"Iosevka Fixed
SS16";color:#212121">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 from<span
class="apple-converted-space"> </span><a
href="https://git.openjdk.org/jfx/pull/1293#discussion_r1411406802"
title="https://git.openjdk.org/jfx/pull/1293#discussion_r1411406802"
moz-do-not-send="true"><span
style="font-family:"Calibri",sans-serif;color:#0078D7">https://git.openjdk.org/jfx/pull/1293#discussion_r1411406802</span></a><span
class="apple-converted-space"> </span>).</span></p>
</div>
</blockquote>
<p>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.</p>
<p>The API is clumsy enough that I loathe creating stylable
properties for the sheer amount of boilerplate that surrounds
them.<br>
</p>
<p>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.<br>
</p>
<blockquote type="cite"
cite="mid:DM5PR1001MB2172D0DA0D52E4ECE5A37C67E581A@DM5PR1001MB2172.namprd10.prod.outlook.com">
<div class="WordSection1">
<p class="MsoNormal" style="caret-color: rgb(33, 33,
33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px"><span style="color:#212121"><o:p></o:p></span></p>
<p class="MsoNormal" style="caret-color: rgb(33, 33,
33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px">
<span style="font-family:"Iosevka Fixed
SS16";color:#212121"> </span><span
style="color:#212121"><o:p></o:p></span></p>
<p class="MsoNormal" style="caret-color: rgb(33, 33,
33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px">
<span style="font-family:"Iosevka Fixed
SS16";color:#212121"> </span><span
style="color:#212121"><o:p></o:p></span></p>
<p class="MsoNormal" style="caret-color: rgb(33, 33,
33);font-variant-caps: normal;orphans:
auto;text-align:start;widows: auto;-webkit-text-stroke-width:
0px;word-spacing:0px">
<span style="font-family:"Iosevka Fixed
SS16";color:#212121">... a few bytes and cpu cycles
would get saved ...<br>
</span></p>
</div>
</blockquote>
<p>This is not for you specifically, but JavaFX has a lot of
"optimizations", some resulting in really questionable patterns
that have/are hurting us:</p>
- Reimplementing core collection classes for some benefit, but then
only partially implementing them (and often buggy), and/or
completely breaking the collection contract [BitSet]<br>
<p>- 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)<br>
</p>
<p>- 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]<br>
</p>
<p>- 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]<br>
</p>
<p>- 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]<br>
</p>
<p>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.</p>
--John
</body>
</html>