-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Concurrency] Add environment variable for disabling async stack slab allocator. #85259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| /// SlabMetadataPtr specifies a fake metadata pointer to place at the beginning | ||
| /// of slab allocations, so analysis tools can identify them. | ||
| template <size_t SlabCapacity, Metadata *SlabMetadataPtr> | ||
| template <size_t SlabCapacity, Metadata *SlabMetadataPtr, bool (*disableSlabAllocator)()> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why introduce the type parameter here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to keep StackAllocator reasonably self-contained and unaware of things like debug variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this encodes it right into the type, so it feels like its even more deeply aware of things like debug variables.
6bd65ba to
3852a97
Compare
|
Very nice, that'll be useful |
compnerd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this seems good. I still thing that this is really over-encoding the environment variable since it encodes it into the type - perhaps we should use the STL approach of _Allocator as a type parameter? That would allow you to hoist the allocator and the knowledge of the environment variable out of the stack allocator itself.
| if (SWIFT_UNLIKELY(EnableSlabAllocator())) { | ||
| free(ptr); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (SWIFT_UNLIKELY(EnableSlabAllocator())) { | |
| free(ptr); | |
| return; | |
| } | |
| if (SWIFT_UNLIKELY(EnableSlabAllocator())) | |
| return free(ptr); |
| Enabled, | ||
| Disabled, | ||
| }; | ||
| static std::atomic<EnableState> savedState = {EnableState::Uninitialized}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be marked constinit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still on C++17, so unfortunately not.
| savedState.store(state, std::memory_order_relaxed); | ||
| } | ||
|
|
||
| return SWIFT_UNLIKELY(state == EnableState::Enabled); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "unlikely" seems backwards.
Does SWIFT_UNLIKELY actually do anything in a return position?
| /// Allocate a memory buffer of \p size. | ||
| void *alloc(size_t size) { | ||
| if (SWIFT_UNLIKELY(EnableSlabAllocator())) | ||
| return malloc(size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have the condition reversed here and below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I switched it from disable to enable and didn't fix up the uses properly. I rearranged various things and it should be correct now.
|
If we expect this to be uncommonly used, it'd be nice to hide it in a path that isn't normally taken. Maybe we can initialize the allocator to ignore the initial slab, and then we only have to check the condition when we'd otherwise be allocating a new slab? There might be a similar unlikely case that we can bury the check during deallocation in, like |
|
@compnerd The |
|
It is mostly a type vs function thing. The type based approach feels more flexible (and is a general pattern in C++). That should allow you to accomplish the same type of isolation but also opens it up for further expansion in the future without changing the type itself. |
|
Gotcha. I'll make that change, then. |
3852a97 to
81107a8
Compare
|
@rjmccall Nice idea about hiding this behind existing conditionals. I pushed changes to make that happen. We now ensure |
rjmccall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, a lot happier about this! I think you've got an UNLIKELY backwards, but maybe it doesn't matter for the code-generation.
| Slab *getSlabForAllocation(size_t size) { | ||
| Slab *slab = (lastAllocation ? lastAllocation->slab : firstSlab); | ||
| if (slab) { | ||
| if (SWIFT_UNLIKELY(slab)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems likely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I blame daylight saving, end of. It actually makes a decent difference in the codegen. Fixing this makes the fast path quite a bit more linear. I was getting a little confused following it around before because it bounced back and forth so much, but didn't quite realize what that meant. Pushed the fix now.
81107a8 to
7e66850
Compare
rjmccall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
|
@swift-ci please test |
7e66850 to
07bbbfc
Compare
|
Now with less build breakage. Needed a little |
|
@swift-ci please test |
07bbbfc to
ecdb965
Compare
|
And |
|
@swift-ci please test |
… allocator. Add SWIFT_DEBUG_ENABLE_TASK_SLAB_ALLOCATOR, which is on by default. When turned off, async stack allocations call through to malloc/free. This allows memory debugging tools to be used on async stack allocations.
ecdb965 to
a62f08e
Compare
|
Someday I will remember to update the symbols in the ABI tests. |
|
@swift-ci please test |
Add SWIFT_DEBUG_ENABLE_TASK_SLAB_ALLOCATOR, which is on by default. When turned off, async stack allocations call through to malloc/free. This allows memory debugging tools to be used on async stack allocations.