Set _StaticBufferBuilder capacity via constructor when merging buffers to optimize memory allocations.

vpenades / SharpGLTF

glTF reader and writer for .NET Standard

MIT License

454 stars 72 forks source link

Set _StaticBufferBuilder capacity via constructor when merging buffers to optimize memory allocations. #166

Closed MeltyPlayer closed 1 year ago

MeltyPlayer commented 1 year ago

I've run into some issues in the past when exporting large GLB/GLTF models in x86 mode, since 32-bit applications only support a max of 2GB of memory. In this PR, I've made some minor optimizations to memory allocations in the GLB/GLTF/OBJ export logic that make it slightly more robust.

Changes:

Initialized the _StaticBufferBuilder in MergeBuffers() with the total capacity of all of the individual buffers. When adding to a common List beyond its capacity, it will double its capacity each time, potentially beyond what is actually needed. This change ensures that the List only uses the exact capacity it needs, making it slightly more robust to out of memory errors.
Adds a new test that generates and saves an arbitrarily large heightmap (1000x1000) to verify that saving large models works for GLB, GLTF, and OBJ.

vpenades commented 1 year ago

Hm.... you're adding multiple features in a single PR, I think it would have been better to propose them in separated PRs.

Some of the optimizations seems good, but I don't like too much tagging the main projects as x86/x64 because it would prevent running the libraries on ARM cpus. If the purpose is testing, the main library projects shoud not be affected in any way.

MeltyPlayer commented 1 year ago

Sounds good, I'll go ahead and remove the x86/x64 changes and split out the other changes.

vpenades commented 1 year ago

Just keep in mind the less files you change, the easier for me to review, and more chances to accept merging the PR

MeltyPlayer commented 1 year ago

Done--I've changed this to just make the buffer change and add the test, and pulled out the WavefrontWriter changes into a separate PR.

vpenades commented 1 year ago

It almost looks fine.

I feel like the memory calculator is not super precise, and I would increase the allocated memory a bit over what's needed, maybe by 1% with something like this:

reservedMemory = reservedMemory + (reservedMemory / 100); // increase by 1%

The other thing I would request is to move the unit tests from Toolkit to ThirdParty tests. There's already an entry for you.

Third party tests help me remember that some features are actually in use by other developers.

MeltyPlayer commented 1 year ago

Sounds good, I've added the reserved memory change and moved the test.