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

[Andy Goryachev]

On Sun, 4 Aug 2024 22:51:33 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> 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 interp...
>
> I think you said that you didn't actually test this, so we don't know if this will perform better. It's based on an assumption of what Java will do, and on assumptions what is "bad" and what is "good".  Without having tested this, you are optimizing blind.  I would be entirely satisfied if you had said "I tested this with a few thousand transitions, the allocation turned out to be a hot spot, so I optimized it which is the reason this code looks a bit out of the ordinary".
> 
> I also would have liked a comment explaining any pitfalls introduced by the optimization; the use of `==` by callers is unusual, and a crucial piece of information when I see code like:
> 
>         if (secondList.isEmpty()) {
>             return firstList.isEmpty() ? firstList : secondList;
>         }
> 
> It would deserve at least an explanation that you're specifically returning one of the passed in arguments so callers can do reference equality checks.
> 
> Java's JIT is very finicky.  For example, an earlier comment asked if the first `if` case is the most common one.  The assumption being that this will improve performance.  However, you can't make that assumption at all (I've done tests that show it's almost impossible to predict which "if order" will be faster on the JVM); re-arranging a function or "unrolling" it can have large negative consequences if that means the JIT will no longer inline your function.
> 
> Now, I will give you that this **probably** will be faster, but I'm making an assumption here... I really can't be sure.  That still leaves some explanatory comments that I think you should add with the assumptions that were made for this optimization.
> 
>>  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.
> 
> No, when hunting for performance problems, it will be readily apparent what total process is taking up all the time, and how often parts of it are being called and how much time they take in total.  Put a profiler on it, and see where the actual time is being spent on a case with a lot of transitions if you want to optimize it.  Even the JVM itself does this, not bothering to even compile code that isn't executed at least a certain amount of times, or not bothering to further optimize code after compilation if that code isn't in a hot path.
> 
> What optimization generally does is it that it makes code harder to understand (almost unavoid...

I think what is being done in this PR is fine, even though your arguments @hjohn are perfectly valid.  One feature that I think has value is the fact that this design tries to limit new allocations, and I think it's a good thing.

One interesting point you bring up is that we have no good performance test for CSS.  What would represent a good performance test?  Create a thousand of labels and measure the time it takes to update?  Does it matter if it's a label or something else?  And do we have a reliable way to measure?  What do you think?

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1705961254