On Sat, 3 Aug 2024 10:04:00 GMT, Michael Strauß <mstra...@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.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702717395
