On Sat, 3 Aug 2024 14:09:19 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>>> Do you think it is preferrable to appeal to JIT magic instead of explicitly
>>> coding the behavior?
>>
>> If the code leads to less questions and is more readable for it, and there
>> is no reason to optimize in the first place (ie. you didn't see a problem),
>> then yes, definitely.
>>
>> Premature optimization only leads to questions by reviewers and future
>> maintainers (they will wonder why the code was optimized, but probably
>> assume there was a good reason for it). Optimized code is harder to reason
>> about and harder to refactor. For example, reading your code I've been left
>> wondering:
>>
>> - Why is it significant to return a specific instance? Are you using `==`
>> to compare these in callers? That's very counter to the nature of Java, and
>> so deserves at a minimum a justification ("performance") and a warning
>> ("careful, callers use `==`")
>> - The extra code paths for `RandomAccess` or not, in an attempt to avoid an
>> iterator allocation (which may not even happen when inlined). Is this even
>> happening in practice (are there any non-`RandomAccesss` lists used?) Is
>> the performance gain worth the extra complexity? Is it worth not having
>> these functions inline into callers, as larger functions mean JIT won't
>> inline them?
>> - Avoiding allocating small arrays; was this a performance problem that
>> would justify writing a function that is now too large to inline? You're
>> trading potential JIT inlining for an almost free heap allocation (Java
>> allocation's are the equivalent of a pointer increment + background GC
>> activity on likely **idling** CPU cores).
>>
>> Would it not have been much better to write the code as simple as possible
>> first, get it passed review well understood by everyone, then maybe,
>> possibly get back to it later with an optimizing PR if there is a
>> performance problem? That last bit is entirely optional and may never
>> happen if it turns out these few transition allocations are but a drop in
>> the bucket of the overall CSS performance.
>
> Sorry if this came across as a bit negative, it's not intended in that way.
> I just think optimizing too early has far more drawbacks than it has
> benefits. In part this is also a reflection on a lot of FX code; some code is
> clearly optimized from a C/C++ developer perspective which not only
> translates poorly to Java, but also makes the code "too dangerous" to modify.
I don't understand this argument at all. Why would it be reasonable to _only_
build a more efficient architecture _if_ there's such a huge problem with the
"naive" alternative that this alone would be readily apparent? If taken to the
extreme, this line of thinking leads to death by a thousand papercuts, no
single issue big enough that it's a problem all by itself, but taken together
it amounts to sluggish performance.
Here's the thing: this code could reasonably be executed tens of thousands of
times per second. Think of a complex user interface that changes its visual
state for many nodes at once. What you seem to perceive as a micro-optimization
is actually a part of an architecture that involves many types in the
`Background` and `Border` area.
The basic idea is: if I can rely on `interpolate()` returning existing
instances (either the start value or the end value) if nothing has changed,
then the "parent object's" interpolate method can use an identity comparison
(which is basically for free) to choose whether to return itself (or the end
value), or return a new instance.
Let's look at the `Border.interpolate()` implementation:
@Override
public Border interpolate(Border endValue, double t) {
Objects.requireNonNull(endValue, "endValue cannot be null");
if (t <= 0) return this;
if (t >= 1) return endValue;
// interpolateListsPairwise() is implemented such that it returns existing
list instances
// (i.e. the 'this.images' or 'endValue.images' arguments) as an indication
that the result
// is shallow-equal to either of the input arguments. This allows us to
very quickly detect
// if we can return 'this' or 'endValue' without allocating a new Border
instance.
List<BorderImage> newImages = images == endValue.images ?
images : InterpolationUtils.interpolateListsPairwise(images,
endValue.images, t);
List<BorderStroke> newStrokes = strokes == endValue.strokes ?
strokes : InterpolationUtils.interpolateListsPairwise(strokes,
endValue.strokes, t);
if (images == newImages && strokes == newStrokes) {
return this;
}
if (endValue.images == newImages && endValue.strokes == newStrokes) {
return endValue;
}
return new Border(newStrokes, newImages);
}
`interpolateListsPairwise()` _already knows_ whether both lists are equal,
which makes the `return this` and `return endValue` branches after the
interpolation basically free. The alternative to this would be to either
pointlessly call `equals()` on a large object graph again and again, or to
always return a new `Border` instance.
This optimization scheme works precisely because all types that are involved
make sure to return existing instances when the result is known to be
interchangable with an existing instance. This is also the reason why your
other suggestion of returning `List.of()` instead of an existing empty list
wouldn't work.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1703242183
