Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Atomic wait: zero-initialize wait list head #2781

Merged
merged 4 commits into from
Jun 20, 2022
Merged

Atomic wait: zero-initialize wait list head #2781

merged 4 commits into from
Jun 20, 2022

Conversation

AlexGuteniev
Copy link
Contributor

Minimum scope of #2755 - based on #2755 (comment) suggestion.

Reduces size of x64 msvcp140_atomic_wait_oss.dll from 44.5 KB to 28.0 KB - did not investigate exactly why, but expect reduction of relocation table and initialized data segment.

Full scope of #2755 as initially suggested can be implemented, but there are some complexities:

  • Fallback path
  • Safe de-initialization

Using lazy initialization for list head

  • Single linked list does not look like a good idea, as there's no efficient removal of the current waiter from list
  • Double linked list that is not head is feasible, but it is more complex that the circular one, and, as a consequence, will take more ops at runtime

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner June 11, 2022 16:33
miscco
miscco approved these changes Jun 11, 2022
View reviewed changes
Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non chained control flow wants a line break but otherwise it looks good to me

stl/src/atomic_wait.cpp Show resolved Hide resolved
stl/src/atomic_wait.cpp Show resolved Hide resolved
stl/src/atomic_wait.cpp Show resolved Hide resolved
@AlexGuteniev @miscco
Apply suggestions from code review
1df8cce 
Co-authored-by: Michael Schellenberger Costa <[email protected]>
@StephanTLavavej StephanTLavavej added the performance Must go faster label Jun 11, 2022
strega-nil-ms
strega-nil-ms approved these changes Jun 16, 2022
View reviewed changes
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment change requested, will just merge it.

stl/src/atomic_wait.cpp Outdated Show resolved Hide resolved
@strega-nil-ms strega-nil-ms removed their assignment Jun 16, 2022
@StephanTLavavej StephanTLavavej self-assigned this Jun 19, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej linked an issue Jun 20, 2022 that may be closed by this pull request
<atomic>: Memory perf: unused 16k buffer in .data when using std::atomic::wait on Windows 8 and later. #2755
Closed
@StephanTLavavej StephanTLavavej merged commit 351efb0 into microsoft:main Jun 20, 2022
@StephanTLavavej
Copy link
Member

Thanks for improving the memory consumption of atomic::wait! ☢️ 🧠 🎉

@AlexGuteniev AlexGuteniev deleted the optimize_wait branch June 20, 2022 05:34
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
@AlexGuteniev @miscco
@strega-nil-ms @fsb4000
Atomic wait: zero-initialize wait list head (microsoft#2781)
09e93c4 
Co-authored-by: Michael Schellenberger Costa <[email protected]>
Co-authored-by: nicole mazzuca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miscco miscco miscco approved these changes

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@strega-nil-ms strega-nil-ms strega-nil-ms approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
performance Must go faster
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

<atomic>: Memory perf: unused 16k buffer in .data when using std::atomic::wait on Windows 8 and later.
4 participants
@AlexGuteniev @StephanTLavavej @miscco @strega-nil-ms