Bug/BufferLeak in ensureLargeEnough/BitMapTextPage

What happens, i have a text displaying the current fps, (always 2-3 digits), however I see a large amount of buffers being generated and trashed after a few seconds (up to 4 per frame wich quickly raise to a few thousand per second). (good that the fix for the tracking is in)

I investigated:



The assemble method generates a mesh with all letters in it.

When the buffer is not large enough a larger fitting one is created (like if the text gets more letters)

The problem here lies in the way the buffer.remaining() works.



Example Output from modificated ensureLargeEnough at bottom of post

Code:
136 but only 128 java.nio.DirectFloatBufferU[pos=0 lim=128 cap=136] Enlarging to 136
As you can clearly see, the buffer is large enough however since remaining respects the set limit, a new buffer with the same size is created and a unneccessary copy happens.
solution:


Sequence of problem:
2 letter text, allocated first time buffer for two letters, everything fine limit set for two letters
text increases to three letters, larger buffer is allocated, limit set for three letters, still fine
text is 2 letters again, old buffer large enough, limit set for two letters
----
text increases again, three letters, however limit was set to two letters, so remaining returns not enough space, a new buffer the same size as the current is created!

This leads to a overloaded gc, wich creates unneccessary out of direct memory crashes, since it is occupied with collection the leaking buffers.
Code:
// increase capacity of buffers as needed fpb.rewind(); fpb = BufferUtils.ensureLargeEnough(fpb, vertCount * 3); fpb.limit(vertCount * 3); pb.updateData(fpb);
Code:
public static FloatBuffer ensureLargeEnough(FloatBuffer buffer, final int required) { if (buffer == null || buffer.remaining() < required) { System.out.println(required + " but only " + buffer.remaining() + " " + buffer); final int position = buffer != null ? buffer.position() : 0; final FloatBuffer newVerts = BufferUtils.createFloatBuffer(position + required); System.out.println("Enlarging to " + buffer.capacity() + " "); if (buffer != null) { buffer.flip(); newVerts.put(buffer); newVerts.position(position); } buffer = newVerts; } return buffer; }
2 Likes

Nice catch.

What are you suggesting to fix this?

i guess “required” should be compared to capacity instead of remaining…

yes either compare that one with capacity, or do a limit(capacity) first, so that callers can rely on remaining returning the correct value after ensureLargeEnough

Hmmm, based on what I can see from reading the code:



Remaining is only correct because the buffer was rewound before entering the method. If the plan is to make it so that ensureLargeEnough can work on partly filled buffers to check the remaining space then limit(capacity) would seem to be the solution.



Otherwise then the check for remaining could just be changed to a check to limit.

Yes, as I see it is meant that I say how much stuff I still want to put in additionally to whatever might already be there. So the limit needs to be adjusted. (Thats the reason it currently works, since the new buffer has limit = capacity initial)







Side point, it would reduce garbage if there would be a helper in the bitmapText where i can say how many lettes I plan to put in.

kinda like new ArrayList(123) vs new ArrayList()



For stuff with many gui on poor devices (androids crappy gc) this might help with loading.

@EmpirePhoenix said:
Side point, it would reduce garbage if there would be a helper in the bitmapText where i can say how many lettes I plan to put in.
kinda like new ArrayList(123) vs new ArrayList()

Mhh yes, good idea.

do you have a test case maybe for the issue? or did you directly discovered that in your game?

directyl discoverd it, but you can easly reproduce it:



It should at most need 2 frames untill the buffer is large enough for 333 and for 22, however according to the tracking the amount of buffers constantly change. (also keep in mind the fps display itself will of course create additional buffers as well since its a bitmaptext also)



[java]

import com.jme3.app.SimpleApplication;

import com.jme3.font.BitmapText;

import com.jme3.util.BufferUtils;



public class EnsureBuggy extends SimpleApplication {



boolean ticktock = false;

private BitmapText test;



@Override

public void simpleInitApp() {

BufferUtils.setTrackDirectMemoryEnabled(true);

this.test = new BitmapText(this.loadGuiFont());

this.test.setText("");

this.test.setLocalTranslation(400, 4100, 10);

this.guiNode.attachChild(this.test);

}



@Override

public void simpleUpdate(final float tpf) {

BufferUtils.printCurrentDirectMemory(null);

if (this.ticktock) {

this.test.setText("22");

} else {

this.test.setText("333");

}

this.ticktock = !this.ticktock;



}



public static void main(final String[] args) {

new EnsureBuggy().start();

}



}



[/java]

Just added a at the top of each ensure.

The buffer leaking stopped, and I dont have any negative sideeffects so far.



[java]

if (buffer != null) {

buffer.limit(buffer.capacity());

}

[/java]

@EmpirePhoenix said:
Side point, it would reduce garbage if there would be a helper in the bitmapText where i can say how many lettes I plan to put in.
kinda like new ArrayList(123) vs new ArrayList()


I do this by creating a BitmapText with a string as large as I want... then it gets set properly when used.

Yeah, that works but is kinda a hack.

@EmpirePhoenix said:
Yeah, that works but is kinda a hack.


Not really, I do it in Swing all the time, too, to get my panels to lay out properly before getting real values. Maybe I'm just used to it but for 90% of my BitmapText I also need to know the size so I start them with some text that is roughly the same as the largest value I expect.

Well any new on this? I have the limit to capactiy in it now and it works fine. However it seem to be not compilant to

http://code.google.com/p/jmonkeyengine/issues/detail?id=504&q=label%3AProduct-jME3&sort=priority&colspec=ID%20Type%20Status%20Component%20Priority%20Product%20Milestone%20Owner%20Summary

Wich says only the limit should be used. So question is is this an exception or not?

@Momoko_Fan, thoughts?

@EmpirePhoenix said:
Well any new on this? I have the limit to capactiy in it now and it works fine. However it seem to be not compilant to
http://code.google.com/p/jmonkeyengine/issues/detail?id=504&q=label%3AProduct-jME3&sort=priority&colspec=ID%20Type%20Status%20Component%20Priority%20Product%20Milestone%20Owner%20Summary
Wich says only the limit should be used. So question is is this an exception or not?

BitmapText is exempt from this requirement since in this case it is a "user" of the system rather than the system itself. I think it is OK to set the limit = capacity in this case. Can you apply the change to core?

I committed the change

Thanks @nehon