On Sun, 4 Aug 2024 22:51:33 GMT, John Hendrikx <[email protected]> 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
