On Sat, 3 Aug 2024 11:43:12 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> There's no performance problem in the sense that my test application runs
>> slowly in any noticeble way. It's one more piece of trying to minimize the
>> allocation of objects in the very common case where nothing has changed.
>>
>> Do you think it is preferrable to appeal to JIT magic instead of explicitly
>> coding the behavior? With your other suggestion of using `switch`, the code
>> looks much cleaner already.
>
>> 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.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702784473
