Use TBB scalable allocator in GMP (windows)
ClosedPublic
Actions

Authored by Erik Abrahamsson (erik85) on May 31 2021, 4:35 AM.

Details

Reviewers

Ray Molenkamp (LazyDodo)
Campbell Barton (campbellbarton)

Commits

rB4f81b4b4ce29: Windows: Use TBBMalloc for GMP

Summary

For more efficient memory management in GMP, TBB's scalable allocator is used as a custom allocator.
In a testfile with a boolean geometry node, this patch uses 32s effective CPU time compared to 52s before.

Code by @Ray Molenkamp (LazyDodo) and cmake additions by me to include tbbmalloc library.

Diff Detail

Repository: rB Blender

Event Timeline

Erik Abrahamsson (erik85) requested review of this revision.May 31 2021, 4:35 AM

Erik Abrahamsson (erik85) created this revision.

Just to clear up why this is needed, tbbmalloc correctly captures al allocations in blender already, however GMP is build with mingw since it doesn't build with msvc, which seemingly tbb has issues hooking into.

There may be something wrong with tbb's proxy initialization, and the culprit may not be mingw.

please hold off on landing this diff, it may or may not be required.

looking into it

The TBB proxiy issue has been resolved, this patch is still required since mingw uses msvcrt.dll for it's crt implementation which tbb doesn't hook, rebuilding gmp to link against ucrtbase.dll did sadly not pan out as finding a compiler that supports that feature at a stable url proved to be challenging

build_files/cmake/platform/platform_win32.cmake
683	`TBB_MALLOC_PROXY_LIBRARIES` is not needed, and somewhat undesirable tbbmalloc_proxy needs some linker shenanigans which are either handled in the tbb headers or in our case in `creator.c` Having these libraries available in a cmake variable may make it look that all you need to do is link them, which is not the case. I'd rather have these not be available in cmake just to put up a barrier to their use.

This revision now requires changes to proceed.Jun 7 2021, 5:28 PM

Diff updated, TBB_MALLOC_PROXY_LIBRARIES removed.

Buildsystem wise, this looks OK now so from that perspective I accept, but I'm not super convinced this code belongs in creator.c , i was "uneasy" when i placed the linker pragma's there, now that it expanded to much more code it feels really out of place.

@Campbell Barton (campbellbarton) would be the one with the strongest opinion on where this belongs, so I'm adding him as blocking reviewer

This looks fine, the defined(WITH_GMP) check should have an explanation for why this is needed (this commit seems useful - https://developer.blender.org/D11435#292585)

Accepting as adding the comment doesn't require extra review iteration.

This revision is now accepted and ready to land.Jun 7 2021, 7:22 PM

Campbell Barton (campbellbarton) added inline comments.Jun 7 2021, 7:27 PM