Implement Smart Pointer Creation With Default Initialization #1315

AdamBucior · 2020-09-24T14:33:54Z

Resolves #48. Based on #778.

stl/inc/memory

StephanTLavavej · 2020-09-24T22:10:18Z

I pushed a commit (deleting an extra newline) with the --author= option, so when we merge this PR, it will be displayed as co-authored by @Weheineman. Thanks for picking up this work!

StephanTLavavej · 2020-10-07T07:48:57Z

Status update: Planning to review this on Thursday, Oct 8. Apologies for the delay!

StephanTLavavej

Thanks! This looks good, my main concern is with a potential corner case in overload resolution that should be avoidable with different metaprogramming. Please let me know if you'd like me to explain further, or I can revise the PR if you want - your preference.

stl/inc/memory

tests/std/tests/P1020R1_smart_pointer_for_overwrite/test.cpp

Co-authored-by: Stephan T. Lavavej <[email protected]>

StephanTLavavej · 2020-10-16T08:07:03Z

This looks great - thanks for the thorough and highly detailed work. (I noticed and appreciated the improvements like using remove_cvref_t on perfectly forwarded _Types to handle const lvalues, and unifying additional repetitive code. Combining 3 overloads into 1, for both the make and allocate families, is awesome.)

I pushed a merge with master and a small set of superficial cleanups, after verifying that the test is still passing:

When granting friendship to _Make_shared_unbounded_array and _Allocate_shared_unbounded_array, drop const on the value parameter size_t _Count. (We conventionally avoid top-level const on value parameters in function declarations, since it has no meaning to callers and is immediately ignored by the language. We are so picky about const! 😹)
Your _Make_shared_unbounded_array/_Allocate_shared_unbounded_array refactoring has a bonus: we no longer need to grant friendship to several make_shared etc. overloads.
When functions take const _Arg&, we don't need to apply remove_cvref_t to _Arg (unless we were worried about _Arg being passed as an explicit template argument, instead of being deduced, which is not a concern here).
Remove a leftover comment after the allocate_shared refactoring.
Use is_bounded_array_v in make_unique_for_overwrite for consistency.

I'll find a second maintainer to review and then we can merge this! 🚀

miscco · 2020-10-16T09:59:32Z

stl/inc/memory

+            _Uninitialized_rev_destroying_backout _Backout{_Out};
+            for (size_t _Idx = 0; _Idx < _Size; ++_Idx) {
+                _Backout._Emplace_back_for_overwrite();
+            }
+            _Backout._Release();


Should we simplify this to

Suggested change

_Uninitialized_rev_destroying_backout _Backout{_Out};

for (size_t _Idx = 0; _Idx < _Size; ++_Idx) {

_Backout._Emplace_back_for_overwrite();

}

_Backout._Release();

_Uninitialized_default_construct_n(_Out, _Size);

There is no such function as _Uninitialized_default_construct_n and we can't use uninitialized_default_construct_n because it doesn't destroy elements in the reverse order.

Hm there is the ranges version (where I found a unneeded specialization that I forgot to remove so thanks ;))

Should we also destroy in backwards order there?

ranges::uninitialized_default_construct_n needs to support forward iterators and destroying in reverse order with them would be hard.

stl/inc/xutility

miscco · 2020-10-16T10:06:40Z

stl/inc/memory

@@ -2351,6 +2369,25 @@ void _Uninitialized_value_construct_multidimensional_n(_Ty* const _Out, const si
    }
 }

+template <class _Ty>
+void _Uninitialized_default_construct_multidimensional_n(_Ty* const _Out, const size_t _Size) {
+    if constexpr (!is_trivially_default_constructible_v<_Ty>) {


To be absolutely sure, even for the meow_overwrite we do not want to default initialize here?

That's the whole point of *_for_overwrite

For better or worse, there are many places in the STL where we assume that we can omit default-initialization of objects that are trivially default constructible. I'm not sure the resulting behavior is portably defined, but it has been working as intended on our implementation. We may need to make changes if and when compilers complain about this trick in constexpr, but that hasn't happened yet and in any case would need for more widespread changes than just this PR.

stl/inc/yvals_core.h

CaseyCarter · 2020-10-16T17:43:27Z

stl/inc/memory

@@ -2351,6 +2369,25 @@ void _Uninitialized_value_construct_multidimensional_n(_Ty* const _Out, const si
    }
 }

+template <class _Ty>
+void _Uninitialized_default_construct_multidimensional_n(_Ty* const _Out, const size_t _Size) {
+    if constexpr (!is_trivially_default_constructible_v<_Ty>) {


For better or worse, there are many places in the STL where we assume that we can omit default-initialization of objects that are trivially default constructible. I'm not sure the resulting behavior is portably defined, but it has been working as intended on our implementation. We may need to make changes if and when compilers complain about this trick in constexpr, but that hasn't happened yet and in any case would need for more widespread changes than just this PR.

StephanTLavavej · 2020-10-16T21:59:53Z

Thanks - the latest changes look good. I was concerned about debug codegen impact from increased use of _Voidify_iter (as we would addressof twice in several cases), but it does centralize the obnoxious casts. So I believe a reasonable compromise is to add a simple throughput optimization, avoiding deref-and-addressof when the input is already a raw pointer, and I went ahead and pushed that. For precedent, we do the same thing in _Unfancy although without if constexpr:

STL/stl/inc/xstddef

Lines 281 to 290 in e7d5113

    
           // FUNCTION TEMPLATE _Unfancy
 
           template <class _Ptrty>
 
           _NODISCARD constexpr auto _Unfancy(_Ptrty _Ptr) noexcept { // converts from a fancy pointer to a plain pointer
 
               return _STD addressof(*_Ptr);
 
           }
 
           template <class _Ty>
 
           _NODISCARD constexpr _Ty* _Unfancy(_Ty* _Ptr) noexcept { // do nothing for plain pointers
 
               return _Ptr;
 
           }

StephanTLavavej · 2020-10-17T06:34:09Z

Thanks again for implementing this feature, @AdamBucior and @Weheineman! This will help users achieve maximum performance with Standard smart pointers. 🚀

Implement P1020R1 and P1973R1

b2dc09e

AdamBucior requested a review from a team as a code owner September 24, 2020 14:33

AdamBucior mentioned this pull request Sep 24, 2020

Smart Pointer Creation With Default Initialization #778

Closed

AdamBucior added 5 commits September 24, 2020 16:44

Unused template parameter pack

bbb74d7

Remove unused alias

8e417f7

Fix test

7733d85

signed/unsigned mismatch

bcfba08

unique_ptr's subscript operator wants size_t

562ae57

miscco reviewed Sep 24, 2020

View reviewed changes

stl/inc/memory Outdated Show resolved Hide resolved

miscco reviewed Sep 24, 2020

View reviewed changes

stl/inc/memory Outdated Show resolved Hide resolved

Code review and warning

e8122e5

CaseyCarter added the cxx20 C++20 feature label Sep 24, 2020

microsoftGH-1315 is based on microsoftGH-778 by @Weheineman.

d57e8a6

StephanTLavavej self-assigned this Sep 30, 2020

StephanTLavavej requested changes Oct 9, 2020

View reviewed changes

TylerMSFT reviewed Oct 9, 2020

View reviewed changes

tests/std/tests/P1020R1_smart_pointer_for_overwrite/test.cpp Outdated Show resolved Hide resolved

TylerMSFT reviewed Oct 9, 2020

View reviewed changes

tests/std/tests/P1020R1_smart_pointer_for_overwrite/test.cpp Outdated Show resolved Hide resolved

TylerMSFT reviewed Oct 9, 2020

View reviewed changes

tests/std/tests/P1020R1_smart_pointer_for_overwrite/test.cpp Outdated Show resolved Hide resolved

StephanTLavavej removed their assignment Oct 9, 2020

AdamBucior and others added 7 commits October 9, 2020 08:53

Apply suggestions from code review

520ccf7

Co-authored-by: Stephan T. Lavavej <[email protected]>

Code review

6a3b07f

clang-format (as always)

5a8ceb8

Use _Voidify_iter

615e044

Fix tests

1a19fe0

clang-format (obviously)

3ab3eeb

Fix tests

f19690e

StephanTLavavej added 5 commits October 15, 2020 23:58

Drop const for value parameters in friend declarations.

2480d7b

Drop unnecessary friendship after refactoring.

c7adba8

Given const _Arg& we don't need remove_cvref_t.

90e2ebc

Remove leftover comment.

cb088ff

Use is_bounded_array_v in make_unique_for_overwrite.

1280bad

StephanTLavavej approved these changes Oct 16, 2020

View reviewed changes

StephanTLavavej removed their assignment Oct 16, 2020

miscco reviewed Oct 16, 2020

View reviewed changes

stl/inc/xutility Outdated Show resolved Hide resolved

miscco reviewed Oct 16, 2020

View reviewed changes

stl/inc/xutility Outdated Show resolved Hide resolved

miscco reviewed Oct 16, 2020

View reviewed changes

Code review

b96db2c

CaseyCarter suggested changes Oct 16, 2020

View reviewed changes

Correct feature-test macro value is 202002L

0334b43

CaseyCarter approved these changes Oct 16, 2020

View reviewed changes

Improve _Voidify_iter debug codegen.

a7a0693

CaseyCarter approved these changes Oct 16, 2020

View reviewed changes

StephanTLavavej approved these changes Oct 16, 2020

View reviewed changes

StephanTLavavej self-assigned this Oct 17, 2020

StephanTLavavej merged commit 1f83628 into microsoft:master Oct 17, 2020

AdamBucior deleted the smart_ptr_for_overwrite branch October 17, 2020 08:03

frederick-vs-ja mentioned this pull request Dec 18, 2023

<memory>: Add different control block types for allocate_shared_for_overwrite #4274

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Smart Pointer Creation With Default Initialization #1315

Implement Smart Pointer Creation With Default Initialization #1315

AdamBucior commented Sep 24, 2020

StephanTLavavej commented Sep 24, 2020

StephanTLavavej commented Oct 7, 2020

StephanTLavavej left a comment

StephanTLavavej commented Oct 16, 2020

miscco Oct 16, 2020

AdamBucior Oct 16, 2020

miscco Oct 16, 2020

AdamBucior Oct 16, 2020

miscco Oct 16, 2020

AdamBucior Oct 16, 2020

CaseyCarter Oct 16, 2020

CaseyCarter Oct 16, 2020

StephanTLavavej commented Oct 16, 2020

StephanTLavavej commented Oct 17, 2020

Implement Smart Pointer Creation With Default Initialization #1315

Implement Smart Pointer Creation With Default Initialization #1315

Conversation

AdamBucior commented Sep 24, 2020

StephanTLavavej commented Sep 24, 2020

StephanTLavavej commented Oct 7, 2020

StephanTLavavej left a comment

Choose a reason for hiding this comment

StephanTLavavej commented Oct 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephanTLavavej commented Oct 16, 2020

StephanTLavavej commented Oct 17, 2020