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

Reduce memory consumption of two ranges tests #2657

Merged
merged 3 commits into from
Apr 27, 2022

Conversation

CaseyCarter
Copy link
Member

By making the stateful lambda a namespace-scope function object (at the suggestion of the FE team) and reducing the combinatorics of the test matrix of range properties.

By making the stateful lambda a namespace-scope function object (at the suggestion of the FE team) and reducing the combinatorics of the test matrix of range properties.
@CaseyCarter CaseyCarter added test Related to test code ranges C++20/23 ranges labels Apr 16, 2022
@CaseyCarter CaseyCarter requested a review from a team as a code owner April 16, 2022 02:31
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.

Thanks a lot, i started to also rely more on struct these days

@CaseyCarter CaseyCarter changed the title Reduce ranges_alg_remove_copy memory Reduce memory consumption of two ranges tests Apr 19, 2022
@AlexGuteniev
Copy link
Contributor

and reducing the combinatorics of the test matrix

Where? 👀

Copy link
Member

@barcharcraz barcharcraz left a comment

Choose a reason for hiding this comment

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

How much time/mem does the struct conversion alone save? Without running only "interesting" specializations?

@StephanTLavavej
Copy link
Member

I have pushed a trivial change to remove unnecessary test:: qualification, which looks more complicated than it is due to (desirable) clang-format reflowing.

@StephanTLavavej
Copy link
Member

@CaseyCarter

and reducing the combinatorics of the test matrix

@AlexGuteniev

Where? 👀

I believe that this is happening, not in the meow_matrix.lst, but in replacing the runtime call to test_in_write<instantiator, const P, P>(); (now guarded by TEST_EVERYTHING) with a few specific lines of instantiator::call.

@CaseyCarter
Copy link
Member Author

CaseyCarter commented Apr 21, 2022

and reducing the combinatorics of the test matrix

Where? 👀

The body of main. The previous body is still there under #ifdef TEST_EVERYTHING. test_in_write:

template <class Instantiator, class Element1, class Element2>
constexpr void test_in_write() {
with_input_ranges<with_writable_iterators<Instantiator, Element2>, Element1>::call();
}

generates calls to instantiator::call with the cross product of "all variations of input range properties" (about 55 cases):

// For all ranges, IsCommon implies Eq.
// For single-pass ranges, Eq is uninteresting without IsCommon (there's only one valid iterator
// value at a time, and no reason to compare it with itself for equality).
Continuation::template call<Args...,
range<input, Element, Sized::no, CanDifference::no, Common::no, CanCompare::no, ProxyRef::no>>();
Continuation::template call<Args...,
range<input, Element, Sized::no, CanDifference::no, Common::no, CanCompare::no, ProxyRef::yes>>();
Continuation::template call<Args...,
range<input, Element, Sized::no, CanDifference::no, Common::yes, CanCompare::yes, ProxyRef::no>>();
Continuation::template call<Args...,
range<input, Element, Sized::no, CanDifference::no, Common::yes, CanCompare::yes, ProxyRef::yes>>();
Continuation::template call<Args...,
range<input, Element, Sized::no, CanDifference::yes, Common::no, CanCompare::no, ProxyRef::no>>();
Continuation::template call<Args...,
range<input, Element, Sized::no, CanDifference::yes, Common::no, CanCompare::no, ProxyRef::yes>>();
Continuation::template call<Args...,
range<input, Element, Sized::no, CanDifference::yes, Common::yes, CanCompare::yes, ProxyRef::no>>();
Continuation::template call<Args...,
range<input, Element, Sized::no, CanDifference::yes, Common::yes, CanCompare::yes, ProxyRef::yes>>();
Continuation::template call<Args...,
range<input, Element, Sized::yes, CanDifference::no, Common::no, CanCompare::no, ProxyRef::no>>();
Continuation::template call<Args...,
range<input, Element, Sized::yes, CanDifference::no, Common::no, CanCompare::no, ProxyRef::yes>>();
Continuation::template call<Args...,
range<input, Element, Sized::yes, CanDifference::no, Common::yes, CanCompare::yes, ProxyRef::no>>();
Continuation::template call<Args...,
range<input, Element, Sized::yes, CanDifference::no, Common::yes, CanCompare::yes, ProxyRef::yes>>();
Continuation::template call<Args...,
range<input, Element, Sized::yes, CanDifference::yes, Common::no, CanCompare::no, ProxyRef::no>>();
Continuation::template call<Args...,
range<input, Element, Sized::yes, CanDifference::yes, Common::no, CanCompare::no, ProxyRef::yes>>();
Continuation::template call<Args...,
range<input, Element, Sized::yes, CanDifference::yes, Common::yes, CanCompare::yes, ProxyRef::no>>();
Continuation::template call<Args...,
range<input, Element, Sized::yes, CanDifference::yes, Common::yes, CanCompare::yes, ProxyRef::yes>>();
with_forward_ranges<Continuation, Element>::template call<Args...>();

and "all variations of writable iterator properties" (about 15 cases):

// Diff and Eq are not significant for "lone" single-pass iterators, so we can ignore them here.
Continuation::template call<Args...,
iterator<input, Element, CanDifference::no, CanCompare::no, ProxyRef::no>>();
Continuation::template call<Args...,
iterator<input, Element, CanDifference::no, CanCompare::no, ProxyRef::yes>>();
with_output_iterators<Continuation, Element>::template call<Args...>();

for a total of 15 * 55 = 825 specializations tested. This is replaced by directly calling instantiator::call five different ways. We know this is adequate coverage only by knowing the details of the implementation.

@CaseyCarter
Copy link
Member Author

CaseyCarter commented Apr 21, 2022

How much time/mem does the struct conversion alone save?

I only measured remove_copy. (@xiangfan-ms reported similar numbers for remove_copy_if which was motivation enough for me to add it without measuring myself.) Only this change affects the maximum memory size, which went from 5.8GB to 125MB. The corresponding reduction in runtime for all 17 test configs on my 3950X was (IIRC) 90 seconds down to the low twenties.

Without running only "interesting" specializations?

This further reduced total runtime down to about 2.5 seconds.

@StephanTLavavej StephanTLavavej self-assigned this Apr 26, 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 merged commit 314f65f into microsoft:main Apr 27, 2022
@StephanTLavavej
Copy link
Member

Thanks for dramatically improving the resource consumption of these tests! 🐱 🎉 📉

@Osyotr
Copy link

Osyotr commented Apr 27, 2022

How could this lambda cause this extensive memory consumption?

@miscco
Copy link
Contributor

miscco commented Apr 27, 2022

How could this lambda cause this extensive memory consumption?

Every time when we instantiate the function, we create a new stateful lambda. And we instantiate that function a ton

@CaseyCarter CaseyCarter deleted the shrink-remove_copy branch August 21, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ranges C++20/23 ranges test Related to test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants