Avoid SmallSetBase heap allocation in default constructor #4884

Squareys · 2022-08-06T20:16:26Z

As described in #4165 (comment), 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.

kripken

Thanks for the PR!

This sounds good to me. Please let's split out the effects.h part though, as suggested in the comments, and then let's benchmark this - I'd like to verify it isn't a significant regression on non-MSVC (but I'm pretty sure it's not).

src/ir/effects.h

src/support/small_set.h

Squareys · 2022-08-08T20:59:56Z

Thanks for the review! I'll have time to apply the requested changes and split the changes from effects.h tomorrow evening CEST.

Squareys · 2022-08-09T14:32:17Z

@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

Squareys · 2022-08-09T15:27:14Z

@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

lgtm % comment

kripken · 2022-08-09T18:08:31Z

src/support/small_set.h

+  // flexible additional storage.
+  // Because std::set allocates the root node of its red-black tree on the
+  // heap, we wrap it std::optional to defer that allocation until we start
+  // using the flexible storage.


This comment is a little specific to the MSVC STL I think. How about this?

Flexible additional storage. We use std::optional here to reduce the overhead in the common case, where the flexible storage is never needed. With std::optional we just have a pointer taking up space, as opposed to a full std::set which is larger. Also, on MSVC's STL at least the std::set constructor does some work, which we want to avoid.

Squareys · 2022-09-28T13:19:31Z

@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.

kripken · 2022-09-28T16:32:32Z

Looks like there are some errors on CI.

Squareys · 2022-11-23T11:41:29Z

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".

Squareys · 2022-11-23T12:14:34Z

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.

kripken · 2022-11-23T18:10:12Z

Code lgtm, also ran a quick local benchmark and looks good. Once tests pass this will merge.

kripken · 2022-11-23T18:28:34Z

Looks like there is a test error though.

Squareys · 2022-11-28T09:33:24Z

Looks like there is a test error though.

Indeed, will find time to have another look before end of the year.

Squareys · 2023-03-28T15:37:16Z

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.

kripken · 2023-03-29T17:13:17Z

I re-ran the workflow now. (It seems like it needs to be approved each time for first-time contributors, unfortunately.)

Signed-off-by: Squareys <squareys@googlemail.com>

…t does Signed-off-by: Squareys <squareys@googlemail.com>

Squareys

@kripken I fixed the test and commented on the two changes I made in the process.

Squareys · 2023-03-29T20:56:34Z

src/support/small_set.h

    // Otherwise we use the fixed storage. Note that if we grow and shrink then
    // we will stay in flexible mode until we reach a size of zero, at which
    // point we return to fixed mode. This is intentional, to avoid a lot of
    // movement in switching between fixed and flexible mode.
-    return flexible.empty();
+    return !flexible || flexible->empty();


The test failure was caused by this condition being faulty.

Squareys · 2023-03-29T20:58:24Z

src/support/small_set.h

@@ -165,23 +172,23 @@ class SmallSetBase {
        // We need to add an item but no fixed storage remains to grow. Switch
        // to flexible.
        assert(fixed.used == N);
-        assert(flexible.empty());


I ended up removing this assert, because it would have been updated to !flexible || flexible->empty(), i.e. equivalent to usingFixed(), which is tested in the condition above. Happy to put it back in, if you believe it's still useful.

kripken · 2023-03-31T21:55:29Z

src/support/small_set.h

-  FlexibleSet flexible;
+  // Flexible additional storage. We use std::optional here to reduce the
+  // overhead in the common case, where the flexible storage is never needed.
+  // With std::optional we just have a pointer taking up space, as opposed to a


Rereading this now, this doesn't seem true at least on linux, as sizeof tells me that the full storage is used. That is, the gnu C++ STL seems to implement an optional as enough space for the item, and a tag whether it is present or not.

Given that, I think we might have been wrong in our previous discussion here. That is, using std::optional here increases memory size, so it may have downsides, and we should probably be more careful.

If our goal is to avoid a problem with the MSVC STL, then a workaround specific to there might make more sense actually.

What I'd suggest immediately though is to perhaps go look at what LLVM's SmallSet does, as a lot of thought has gone into that. We should probably do what they are doing.

Unfortunately, LLVM's SmallSet doesn't seem to address the heap allocation on MSVC.

I believe the measurements you had made were on Linux? It seemed benefits from using avoiding the std::set construction outweighed the size increase there, too? (At least when used in EffectsAnalyzer, and way less significant than on windows... I suppose it's not making heap allocations in the STL implementation there).

Effectively, we could, of course, avoid the entire PR, if we'd use another STL, as you had suggested in this comment.

Squareys force-pushed the small-set-heap-allocation branch 3 times, most recently from 0afc9bd to abf24ad Compare August 6, 2022 20:55

kripken reviewed Aug 8, 2022

View reviewed changes

src/ir/effects.h Outdated Show resolved Hide resolved

src/support/small_set.h Outdated Show resolved Hide resolved

src/support/small_set.h Outdated Show resolved Hide resolved

Squareys force-pushed the small-set-heap-allocation branch from abf24ad to 6c98021 Compare August 9, 2022 14:16

Squareys mentioned this pull request Aug 9, 2022

Use SmallSet in EffectsAnalyzer #4885

Open

kripken approved these changes Aug 9, 2022

View reviewed changes

kripken reviewed Aug 9, 2022

View reviewed changes

Squareys force-pushed the small-set-heap-allocation branch from 6c98021 to 2a05446 Compare September 28, 2022 13:18

Squareys force-pushed the small-set-heap-allocation branch from 2a05446 to ff55997 Compare November 23, 2022 11:37

Squareys force-pushed the small-set-heap-allocation branch 2 times, most recently from c7c7201 to 7725c70 Compare November 23, 2022 12:51

kripken approved these changes Nov 23, 2022

View reviewed changes

kripken enabled auto-merge (squash) November 23, 2022 18:11

auto-merge was automatically disabled March 28, 2023 15:35
Head branch was pushed to by a user without write access

Squareys force-pushed the small-set-heap-allocation branch from 7725c70 to b150937 Compare March 28, 2023 15:35

Squareys added 2 commits March 29, 2023 22:55

Avoid some copies in SmallSetBase(std::initializer_list<T>)

b98f9f2

Signed-off-by: Squareys <squareys@googlemail.com>

Avoid std::set heap allocation in default constructor of SmallSetBase()

e9f5ac4

Signed-off-by: Squareys <squareys@googlemail.com>

Return number of removed elements from SmallSet::erase() like std::se…

fe060b8

…t does Signed-off-by: Squareys <squareys@googlemail.com>

Squareys force-pushed the small-set-heap-allocation branch from b150937 to fe060b8 Compare March 29, 2023 20:55

Squareys commented Mar 29, 2023

View reviewed changes

kripken reviewed Mar 31, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Avoid SmallSetBase heap allocation in default constructor #4884

Avoid SmallSetBase heap allocation in default constructor #4884

Uh oh!

Squareys commented Aug 6, 2022

kripken left a comment

Uh oh!

Uh oh!

Uh oh!

Squareys commented Aug 8, 2022

Squareys commented Aug 9, 2022

Squareys commented Aug 9, 2022

kripken left a comment

kripken Aug 9, 2022 •

edited

Loading

Squareys commented Sep 28, 2022

kripken commented Sep 28, 2022

Squareys commented Nov 23, 2022

Squareys commented Nov 23, 2022

kripken commented Nov 23, 2022

kripken commented Nov 23, 2022

Squareys commented Nov 28, 2022

Squareys commented Mar 28, 2023

kripken commented Mar 29, 2023

Squareys left a comment

Squareys Mar 29, 2023

Squareys Mar 29, 2023

kripken Mar 31, 2023

Squareys Apr 3, 2023 •

edited

Loading

Avoid SmallSetBase heap allocation in default constructor #4884

Are you sure you want to change the base?

Avoid SmallSetBase heap allocation in default constructor #4884

Uh oh!

Conversation

Squareys commented Aug 6, 2022

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Squareys commented Aug 8, 2022

Squareys commented Aug 9, 2022

Squareys commented Aug 9, 2022

kripken left a comment

Choose a reason for hiding this comment

kripken Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Squareys commented Sep 28, 2022

kripken commented Sep 28, 2022

Squareys commented Nov 23, 2022

Squareys commented Nov 23, 2022

kripken commented Nov 23, 2022

kripken commented Nov 23, 2022

Squareys commented Nov 28, 2022

Squareys commented Mar 28, 2023

kripken commented Mar 29, 2023

Squareys left a comment

Choose a reason for hiding this comment

Squareys Mar 29, 2023

Choose a reason for hiding this comment

Squareys Mar 29, 2023

Choose a reason for hiding this comment

kripken Mar 31, 2023

Choose a reason for hiding this comment

Squareys Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

kripken Aug 9, 2022 •

edited

Loading

Squareys Apr 3, 2023 •

edited

Loading