binaryen
binaryen copied to clipboard
WebAssembly
Avoid SmallSetBase heap allocation in default constructor
Hi @kripken & @sbc100 !
As described in https://github.com/WebAssembly/binaryen/issues/4165#issuecomment-1176486775, this is the change that wraps the std::set in the SmallSetBase into a std::optional to avoid the heap allocation that creates the red-black-tree root on MSVC STL.
I made one drive-by change in a separate commit that avoids a copy per element in the for-each of the std::initializer_list constructor of SmallSetBase.
Best, Jonathan from Wonderland Engine.
Thanks for the review! I'll have time to apply the requested changes and split the changes from effects.h tomorrow evening CEST.
@kripken I applied the feedback and split off the changes to effects.h 👍 Opening a PR for that in a few moments, benchmarking a few alternatives to 10
@kripken Please consider the benchmark and analysis here, on the other PR before merging this.
While it's a net-positive, it does pessimize the code without the EffectsAnalyzer change, which seems to be a special case that benefits massively from avoiding the allocation, possibly allocated especially often.
@kripken Changed the comment and rebased. Sorry for the crazy long delay!
I'll be rebasing #4885 next, which is waiting for this to land.
Rebased onto latest main and fixed the failing test case.
Sorry for the crazy slow iteration cycle times on this, this project was done during a once-per-month "Workflow Improvement Day".
Note, while rebasing #4885, I noticed, that SmallSet::erase() was returning void instead of size_t like std::set does. I added a commit to update that, such that the code which depends on it in effects analyzer compiles.
Code lgtm, also ran a quick local benchmark and looks good. Once tests pass this will merge.
Looks like there is a test error though.
Indeed, will find time to have another look before end of the year.
end of the year.
Oh well... sorry this is taking so long.
It's not a test failure, but likely a clang or GCC compiler difference, so I'd appreciate if you could approve the workflow as the previous logs expired. I just rebased to trigger the workflow run.
I re-ran the workflow now. (It seems like it needs to be approved each time for first-time contributors, unfortunately.)