Gamua / Starling-Framework

The Cross Platform Game Engine
http://www.starling-framework.org
Other
2.82k stars 821 forks source link

Replacing ByteArray.clear with ByteArray.length = 0 to save memory allocation #1027

Closed hardcoremore closed 6 years ago

hardcoremore commented 6 years ago

https://github.com/Gamua/Starling-Framework/blob/f44c2651c90ff95cb84efd0b82659baf7860973f/starling/src/starling/rendering/VertexData.as#L170

Hey Daniel,

Can we here set ByteArray.length = 0 instead of clear because clear() deallocates memory and GC is affected by it. Instead let just use ByteArray.clear() when display object is disposed only. I have also used ByteArray.length = 0 in VertexData.format() function.

I got 1000 allocations less when I use ByteArray.length = 0 vs ByteArray.clear()

What do you think?

PrimaryFeather commented 6 years ago

Hm, a quick check shows that this method is only used by Mesh.dispose(), in which case I wanted to make sure the memory is released right away. But I actually thought this would release the memory without requiring the GC — maybe that's not really the case.

In any case: just changing the length to zero would mean that the ByteArray would keep taking up as much memory as before; this doesn't free up any memory. And what I wanted to do with the clear method is provide a way to do just that. Otherwise, one can always call vertexData.numVertices = 0.

What I could do, though, is remove the call to clear inside MeshBatch.dispose and VertexData.set format. But I'd like to make some Scout tests myself first.

hardcoremore commented 6 years ago

Daniel my mistake. I tagged a line in a VertexData.clear() method where I actually thought on trim method. I agree that ByteArray.clear() should be called in VertexData.clear() but not in VertexData.trim().

I was actually thinking to remove calling ByteArray.clear() in VertexData.trim() and VertexData.format() methods only. And in IndexData.trim() method as well.

Thanks again.

PrimaryFeather commented 6 years ago

I agree that it should probably be removed in the VertexData.format method, but in trim, it can't be removed. The whole idea of trim is to let the ByteArray only take up exactly as much memory as it requires (remove any overhead), and this can unfortunately only be done through the clear method. :-/

But anyway, trim is only called very rarely in Starling, so I don't suppose that one's a problem. The format setter is probably what's really causing the issues.

hardcoremore commented 6 years ago

Great than. Lets remove it for format and see if that reduces allocations.

PrimaryFeather commented 6 years ago

There is one side effect, though — and that's the reason I was trimming in the first place:

When you assign a MeshFormat to a Mesh that has additional vertex attributes (which will be almost all mesh styles), this will make the VertexData blow up to 4k of memory - even if it's just 4 vertices (i.e. one quad). The reason for this is explained [here](http://doc.starling-framework.org/current/starling/rendering/VertexData.html#VertexData()) and in more detail in the Starling Handbook.

The best solution will probably be to avoid clear when possible, but do it if it makes sense. I'll give it a try!

PrimaryFeather commented 6 years ago

Please try out that version — it should avoid some of those (de-) allocations.

hardcoremore commented 6 years ago

This is fine now. Thanks Daniel!

Although IndexData.trim() is really called very rarely on the other hand VertexData.trim() is called a lot when I play my level.

PrimaryFeather commented 6 years ago

I'm glad to hear that! Again, thanks for the heads-up! I'll close this issue then.