RFR: 8332895: Support interpolation for backgrounds and borders [v3]

[Michael Strauß]

On Sat, 3 Aug 2024 14:09:19 GMT, John Hendrikx <jhendrikx at 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