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.

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

<generator>: An attempt to merge _Top and _Info #4619

Conversation

JMazurkiewicz
Copy link
Contributor

Merge promise_type::_Top and promise_type::_Info into single promise_type::_Data member of type uintptr_t.

@JMazurkiewicz JMazurkiewicz requested a review from a team as a code owner April 23, 2024 12:30
@StephanTLavavej StephanTLavavej added performance Must go faster generator C++23 generator labels Apr 23, 2024
@StephanTLavavej
Copy link
Member

/azp run STL-CI

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephanTLavavej StephanTLavavej self-assigned this Apr 23, 2024
@StephanTLavavej
Copy link
Member

For my own education, I'd like to confirm - the size of promise_type is important enough to be worth this additional complexity?

@JMazurkiewicz
Copy link
Contributor Author

For my own education, I'd like to confirm - the size of promise_type is important enough to be worth this additional complexity?

It's debatable, that's why I called this PR An attempt.

My opinion is yes - it is important. Since promise object is dynamically allocated, smaller size can be useful when users supply their own allocator and there is not a lot of memory to give. At first it seems that saving 4/8 bytes is not important, but when you think about recursive case, then we can gain much more. Also, the additional complexity is negligible in non-recursive case - for example, a call to _Get_top should be optimized away in /O1 build (I'm going to verify it).

Maybe we should mark this PR as decision needed?

@StephanTLavavej
Copy link
Member

Being dynamically allocated is compelling enough to me, thanks!

stl/inc/generator Outdated Show resolved Hide resolved
stl/inc/generator Outdated Show resolved Hide resolved
stl/inc/generator Outdated Show resolved Hide resolved
stl/inc/generator Outdated Show resolved Hide resolved
stl/inc/generator Outdated Show resolved Hide resolved
stl/inc/generator Show resolved Hide resolved
@StephanTLavavej StephanTLavavej merged commit 0191658 into microsoft:feature/generator May 10, 2024
39 checks passed
@JMazurkiewicz JMazurkiewicz deleted the generator/merge-members branch May 14, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generator C++23 generator performance Must go faster
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants