Changes pipeline lists to from LinkedList to ArrayList

[codex128]

As pointed out in #2530, pipelines and contexts use LinkedLists where ArrayLists could be used for better performance. This PR changes usedContexts and usedPipelines to ArrayLists and removes iterator creation during pipeline and context cleanup.

[github-actions]

🖼️ Screenshot tests have failed.

The purpose of these tests is to ensure that changes introduced in this PR don't break visual features. They are visual unit tests.

📄 Where to find the report:

• Go to the (failed run) > Summary > Artifacts > screenshot-test-report
• Download the zip and open jme3-screenshot-tests/build/reports/ScreenshotDiffReport.html

⚠️ If you didn't expect to change anything visual:
Fix your changes so the screenshot tests pass.

✅ If you did mean to change things:
Review the replacement images in jme3-screenshot-tests/build/changed-images to make sure they really are improvements and then replace and commit the replacement images at jme3-screenshot-tests/src/test/resources.

✨ If you are creating entirely new tests:
Find the new images in jme3-screenshot-tests/build/changed-images and commit the new images at jme3-screenshot-tests/src/test/resources.

Note; it is very important that the committed reference images are created on the build pipeline, locally created images are not reliable. Similarly tests will fail locally but you can look at the report to check they are "visually similar".

See https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-screenshot-tests/README.md for more information

Contact @richardTingle (aka richtea) for guidance if required

[richardTingle]

Thanks for the performance improvements!

If you merge master into this branch (or do anything that causes a new pipeline to run) the tests should now pass

[codex128]

@richardTingle unfortunately screenshot tests are still failing. Like before, the differences aren't visually detectable.

[richardTingle]

@codex128 I had always assumed PR pipelines ran against the proposed merged code but that doesn't look to be the case. I can see logs from the old pipeline here. Could you try merging master into your branch and seeing what happens?

[richardTingle]

Ah, I meant the other way round, merge master into this branch (and then the pipeline would rerun and pass). But it looks like the master pipeline has passed so all good!

[codex128]

Oops, My bad. Good to hear it's passing now!

[pspeed42]

Is this just a memory micro-optimization? What we gain in not creating an iterator every time, I wonder if we lose in two new method calls and a lack of 'mutation protection'.

At least on desktop java, iterators are 'young generation' and are essentially like stack allocations in other languages. I assume that Android has gotten better in the intervening decade that this was a problem.

Else, maybe use SafeArrayList and iterate over the array... then at least there can never be accidental infinite loops, etc... and we would also likely avoid an index out of bounds check. (I mean... if we're micro-optimizing anyway.)

[codex128]

Ok, your doubts motivated me to research this more thoroughly. From Effective Java by Joshua Bloch:

The for-each loop, introduced in release 1.5, gets rid of the clutter and the opportunity for error by hiding the iterator or index variable completely. The resulting idiom applies equally to collections and arrays:

for (Element e : elements) {
    doSomething(e);
}

When you see the colon (:), read it as “in.” Thus, the loop above reads as “for each element e in elements.” Note that there is no performance penalty for using the for-each loop, even for arrays. In fact, it may offer a slight performance advantage over an ordinary for loop in some circumstances, as it computes the limit of the array index only once. While you can do this by hand (Item 45), programmers don’t always do so.

On the other hand, I did see a good number articles claiming that simple for-loops performed roughly twice as fast as iterators on micro-benchmarks. I didn't see anything about iterators being stack-allocated, and I would assume they're heap-allocated.

[pspeed42]

The bug here is that if endContextRenderFrame() adds a new context then this loop will go forever and ever. That's why it's a bad pattern. A for-each will throw an exception in this case. (SafeArrayList iterating over the array will be looking at a frozen-in-time version and allow the safe iteration... which is the 'safe' in SafeArrayList.)

Another issue with this pattern that JME used to hit A LOT is when the inner loop deletes something from the list and then skips a value. It used to have a bunch of backwards loops trying to fix the problem... until we implemented SafeArrayList.

From a performance perspective, we've added a per-loop call to size() and a per-loop call to get() that otherwise might have been unnecessary. (The Iterator implementation may be doing something more efficient than what 'get()' is doing.)

If elements was an array then the for-each form can even avoid the per-element index out of bounds check.

re: "I didn't see anything about iterators being stack-allocated" They aren't. Java doesn't support such a thing. But what it DOES support is fast-deallocation of the "young generation". This means that for super-temporary objects like iterators, they barely stick around and are essentially free to deallocate. We only pay for the constructor.

The other bad thing about the for( int i= 0; ... ) form is that if Java ever does something smarter in the future with for-each then this code will not be taking advantage of it. It's tricking Java into thinking it's doing something else.

Curious what the reasoning behind switching out LinkedList for ArrayList. Is it to avoid the internal node allocation? Was it so that a get(index) style loop would perform better?

[codex128]

I see what you mean now. In that case we should probably revert this... @riccardobl what do you think?

Curious what the reasoning behind switching out LinkedList for ArrayList.

My understanding is that LinkedLists have trouble with random accesses, which make them slower than ArrayList in most cases. That and internal node allocations, as you said.

[riccardobl]

I agree with using SafeArrayList.

But regarding the rest, I'd prefer to not rely too much on jvm optimizations in the render loop, because in the modern java ecosystem we are starting to have different jvms that behave differently, so imo, since this is the part that is more sensitive in the engine, we should keep gc pressure to the very minimum.

[riccardobl]

SafeArrayList would have no GC pressure at all to use a for-each loop.

Indeed, iterating over the internal array of SafeArrayList is probably the winner here.
Assuming it reasonable to believe someone might end up changing the list from endRenderFrame or endContextRenderFrame, i haven't look enough into the pipeline code yet, to make this call, but @codex128 will know

[pspeed42]

If the list changes less often than it is iterated, for-each over SafeArrayList.getArray() is an easy winner. (It can still be a winner otherwise, but if you iterate over it more than you modify it then it's an EASY winner.)

...nothing will ever be faster than for-each over an array. There will be no iterator allocation. It's all wins.

The safety aspects are just extra icing.

[codex128]

Assuming it reasonable to believe someone might end up changing the list from endRenderFrame or endContextRenderFrame.

It's not an unreasonable assumption, especially given that both lists are class members. Really, both lists should be local, except that would require changing renderViewPort's signature.

If the list changes less often than it is iterated, for-each over SafeArrayList.getArray() is an easy winner. (It can still be a winner otherwise, but if you iterate over it more than you modify it then it's an EASY winner.)

Both lists are iterated once per frame, and added to (at most) once per viewport per frame. So a bit more on the modification side.

[pspeed42]

So sometimes they are not modified per frame? But they are always iterated once per frame?

[codex128]

Sorry, I was unclear. All unique used pipelines are added to one of the lists each frame (that is, between one and the number of viewports are added). The other list is the same way, except with pipeline contexts. Both lists are iterated exactly once per frame.