Skip to content

Changes pipeline lists to from LinkedList to ArrayList #2543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 11, 2025
+6 −6
Merged

Changes pipeline lists to from LinkedList to ArrayList #2543

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions jme3-core/src/main/java/com/jme3/renderer/RenderManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ public class RenderManager {
private final ArrayList<ViewPort> viewPorts = new ArrayList<>();
private final ArrayList<ViewPort> postViewPorts = new ArrayList<>();
private final HashMap<Class, PipelineContext> contexts = new HashMap<>();
private final LinkedList<PipelineContext> usedContexts = new LinkedList<>();
private final LinkedList<RenderPipeline> usedPipelines = new LinkedList<>();
private final ArrayList<PipelineContext> usedContexts = new ArrayList<>();
private final ArrayList<RenderPipeline> usedPipelines = new ArrayList<>();
private RenderPipeline defaultPipeline = new ForwardPipeline();
private Camera prevCam = null;
private Material forcedMaterial = null;
Expand Down Expand Up @@ -1367,11 +1367,11 @@ public void render(float tpf, boolean mainFrameBufferActive) {
}

// cleanup for used render pipelines and pipeline contexts only
for (PipelineContext c : usedContexts) {
c.endContextRenderFrame(this);
for (int i = 0; i < usedContexts.size(); i++) {
Copy link
Contributor

@pspeed42 pspeed42 Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

@codex128 codex128 Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@pspeed42 pspeed42 Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@codex128 codex128 Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@riccardobl riccardobl Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@riccardobl riccardobl Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@pspeed42 pspeed42 Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@codex128 codex128 Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@pspeed42 pspeed42 Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@codex128 codex128 Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

usedContexts.get(i).endContextRenderFrame(this);
}
for (RenderPipeline p : usedPipelines) {
p.endRenderFrame(this);
for (int i = 0; i < usedPipelines.size(); i++) {
usedPipelines.get(i).endRenderFrame(this);
}
usedContexts.clear();
usedPipelines.clear();
Expand Down
Loading