Reduce stack space consumption of list<T>::insert #366

BillyONeal · 2019-12-10T04:29:34Z

Description

This change fixes _List_node_insert_op to avoid consuming otherwise unused stack space for a T.

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

Identifiers in product code changes are properly _Ugly as per
https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
The STL builds successfully and all tests have passed (must be manually
verified by an STL maintainer before automated testing is enabled on GitHub,
leave this unchecked for initial submission).
These changes introduce no known ABI breaks (adding members, renaming
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.).
These changes were written from scratch using only this repository,
the C++ Working Draft (including any cited standards), other WG21 papers
(excluding reference implementations outside of proposed standard wording),
and LWG issues as reference material. If they were derived from a project
that's already listed in NOTICE.txt, that's fine, but please mention it.
If they were derived from any other project (including Boost and libc++,
which are not yet listed in NOTICE.txt), you must mention it here,
so we can determine whether the license is compatible and what else needs
to be done.

…olves #973579 and microsoftGH-365.

BillyONeal

I'm interested if there are much better ideas that avoid a branch on every iteration or the increased code size from calling the ctor twice.

BillyONeal · 2019-12-10T04:30:40Z

stl/inc/forward_list

        _Alloc_construct_ptr<_Alnode> _Newnode(_Al);
+        if (_Tail == pointer{}) {
+            _Newnode._Allocate(); // throws
+            _Alnode_traits::construct(_Al, _STD addressof(_Newnode._Ptr->_Myval), _Carg...); // throws


This repeated constructor has a somewhat negative code size cost.

BillyONeal · 2019-12-10T04:31:09Z

stl/inc/forward_list

+            const auto _Newhead = _Newnode._Release();
+            _Head               = _Newhead;
+            _Tail               = _Newhead;
+            ++_First; // throws


Previously we forgot to handle an iterator that threw in its op++ nicely.

Using operator++ throws and nicely in a sentence...

BillyONeal · 2019-12-10T04:31:28Z

stl/inc/list

        _Alloc_construct_ptr<_Alnode> _Newnode(_Al);
+        if (_Added == 0) {
+            _Newnode._Allocate(); // throws
+            _Alnode_traits::construct(_Al, _STD addressof(_Newnode._Ptr->_Myval), _Carg...); // throws


Ditto unfortunate code duplication

BillyONeal · 2019-12-10T04:32:11Z

stl/inc/list

+        if (_Added == 0) {
+            _Newnode._Allocate(); // throws;
+            _Alnode_traits::construct(_Al, _STD addressof(_Newnode._Ptr->_Myval), *_First); // throws
+            const auto _Newhead = _STD exchange(_Newnode._Ptr, pointer{});


The null assignment moved into the loop to handle throwing _First++.

StephanTLavavej · 2019-12-10T04:46:45Z

stl/inc/list

-    _List_node_insert_op(const _List_node_insert_op&) = delete;
-    _List_node_insert_op& operator=(const _List_node_insert_op&) = delete;
+    _List_node_insert_op2(const _List_node_insert_op2&) = delete;
+    _List_node_insert_op2& operator=(const _List_node_insert_op2&) = delete;

    template <class... _CArgT>
    void _Append_n(size_type _Count, const _CArgT&... _Carg) {
        // Append _Count Ts constructed from _Carg to this insert operation.


Pre-existing, but we usually avoid saying "Ts" since nothing is actually named T in this implementation. Can this say "elements"?

How about "values"? (Since containers have value_type, not element_type)

We always speak of containers having elements. The distinction is critical, beyond the syntax of value_type. The difference is that with a list<int>, an int value might be provided (e.g. in push_back), but an int element is physically owned by the container.

Technically they aren't owned by a container yet here but they will be soon :). Will use elements.

StephanTLavavej · 2019-12-10T04:51:03Z

stl/inc/list

        _Alloc_construct_ptr<_Alnode> _Newnode(_Al);
-        for (; _First != _Last; ++_First) {
+        if (_Added == 0) {
+            _Newnode._Allocate(); // throws;


Spurious semicolon in comment.

StephanTLavavej · 2019-12-10T04:54:08Z

stl/inc/list

+            _Construct_in_place(_Newnode._Ptr->_Prev, _Local_tail);
+            _Construct_in_place(_Local_head->_Prev, _Newnode._Ptr);
+            _Construct_in_place(_Local_tail->_Next, _Newnode._Ptr);
+            _My_data._Mysize = _Local_added;


Because this is "assumed nothrow hereafter", I observe that _My_data._Mysize = 0; on 724 and _My_data._Mysize = _Local_added; here could be extracted.

Ah, in this case it's no longer an assumption because we are no longer doing allocator::construct on pointers. And yes, will extract.

StephanTLavavej · 2019-12-10T04:55:59Z

stl/inc/forward_list


    template <class... _CArgT>
-    pointer _Append_n(typename _Alnode_traits::size_type _Count, const _CArgT&... _Carg) {
+    void _Append_n(typename _Alnode_traits::size_type _Count, const _CArgT&... _Carg) {
        // Append _Count copies of T, constructed from _Carg, to this insert-after operation


Pre-existing, can we avoid saying "T" when there is no such name?

StephanTLavavej · 2019-12-10T04:57:47Z

stl/inc/forward_list

    }

-    void _Attach_after(pointer _After) noexcept {
+    pointer _Attach_after(pointer _After) noexcept {
        // Attaches the values in this insert operation after _After, and resets *this to the default initialized state


Pre-existing, hyphenate "default-initialized"?

StephanTLavavej · 2019-12-10T04:58:27Z

stl/inc/forward_list

        // Attaches the values in this insert operation after _After, and resets *this to the default initialized state
+        auto _Local_tail = _Tail;


Can _Local_tail be const?

miscco · 2019-12-10T07:54:33Z

stl/inc/forward_list

            _Newnode._Allocate(); // throws
            _Alnode_traits::construct(_Al, _STD addressof(_Newnode._Ptr->_Myval), *_First); // throws
-            _Construct_in_place(*_Tail, _Newnode._Ptr); // nothrow
-            _Tail = _STD addressof(_Newnode._Ptr->_Next);
+            const auto _Newhead = _Newnode._Release();


There is an implementation divergence between _Append_n and this function in that additional temporary. I could not find the reason for that.

--_Count can't throw, so we can avoid a bunch of nullptr assignments, but ++_First can throw, so we need to deactivate the _Alloc_construct_ptr after ownership of each element has been transferred to *this.

Thanks for the clarification. We need a “that is correct and I am deeply horrified“ emoji

The eyes are close enough right?

Nah, depending on the browser they can look cute. 😱 seems appropriate

BillyONeal · 2019-12-10T08:36:25Z

stl/inc/list

-        return _First_inserted;
+        _My_data._Mysize += _Local_added;
+        _Added = 0;
+        _Head  = pointer{};


Don't need to zero these out since _Added == 0 already marks it.

BillyONeal added 2 commits December 9, 2019 20:26

Avoid burning unused stack space for a T in _List_node_insert_op. Res…

083a706

…olves #973579 and microsoftGH-365.

Change forward_list to follow a similar pattern for consistency.

1fd1109

BillyONeal requested a review from a team as a code owner December 10, 2019 04:29

BillyONeal commented Dec 10, 2019

View reviewed changes

StephanTLavavej approved these changes Dec 10, 2019

View reviewed changes

miscco reviewed Dec 10, 2019

View reviewed changes

BillyONeal commented Dec 10, 2019

View reviewed changes

First round of code review feedback.

c803ecc

BillyONeal self-assigned this Dec 11, 2019

BillyONeal requested a review from StephanTLavavej December 11, 2019 00:43

StephanTLavavej approved these changes Dec 11, 2019

View reviewed changes

BillyONeal requested a review from CaseyCarter December 11, 2019 03:14

BillyONeal merged commit 991ffe5 into microsoft:master Dec 11, 2019

BillyONeal deleted the stack_space branch December 11, 2019 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce stack space consumption of list<T>::insert #366

Reduce stack space consumption of list<T>::insert #366

BillyONeal commented Dec 10, 2019

BillyONeal left a comment

BillyONeal Dec 10, 2019

BillyONeal Dec 10, 2019

miscco Dec 10, 2019

BillyONeal Dec 10, 2019

BillyONeal Dec 10, 2019

StephanTLavavej Dec 10, 2019

BillyONeal Dec 10, 2019

StephanTLavavej Dec 10, 2019

BillyONeal Dec 10, 2019

StephanTLavavej Dec 10, 2019

StephanTLavavej Dec 10, 2019

BillyONeal Dec 10, 2019

StephanTLavavej Dec 10, 2019

StephanTLavavej Dec 10, 2019

StephanTLavavej Dec 10, 2019

miscco Dec 10, 2019

BillyONeal Dec 10, 2019

miscco Dec 10, 2019

BillyONeal Dec 10, 2019

miscco Dec 10, 2019

BillyONeal Dec 10, 2019

		// Attaches the values in this insert operation after _After, and resets *this to the default initialized state
		auto _Local_tail = _Tail;

Reduce stack space consumption of list<T>::insert #366

Reduce stack space consumption of list<T>::insert #366

Conversation

BillyONeal commented Dec 10, 2019

Description

Checklist

BillyONeal left a 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

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

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