Optimize std::vector<:ghost:> copy algorithms #2461

miscco · 2022-01-07T12:23:33Z

This adds optimizations for std::vector<:ghost:> to make it a bit more useful.

I also added a new subproject for benchmarks. I tried to make it as clean as possible but my CMake skills are limited.

There is one big wrinkle regarding benchmarks in that I currently use an installed version of benchmark. I am not sure if it is possible to link against our produced libraries without any linker errors. As I am currently using header only code I postponed adding benchmark as a subproject for another time.

I only added the x64-Release target for now and will wait until there is a decision on how to name things before adding other targets.

For posterity, I also did some experimentations around using lit for generating and running the benchmarks. But the result was really ugly and also a bit unwieldly as I was not able to make it print out the results of a passed test.

I am also not sure whether there is really value added in testing different standard modes. However, clang vs MSVC would definitely be worthwile

miscco · 2022-01-07T13:27:35Z

Here are the benchmark results
Current results:

With this PR

benchmarks/std/containers/sequences/vector.bool/copy/test.cpp

benchmarks/std/main.cpp

benchmarks/std/containers/sequences/vector.bool/move/test.cpp

benchmarks/std/containers/sequences/vector.bool/copy/test.cpp

benchmarks/std/containers/sequences/vector.bool/copy_n/test.cpp

AlexGuteniev · 2022-01-07T14:40:07Z

benchmarks/std/containers/sequences/vector.bool/copy_n/test.cpp

+    static random_device rd;
+    static mt19937 gen{random_device{}()};
+    vector<bool> result(size);
+    generate_n(result.begin(), size, []() { return bernoulli_distribution{0.5}(gen); });


unnecessary ()

I am not sure, I thought that this is only C++20 and I would want to be able to run the tests in earlier standard modes too

It has been there since lambdas introduction: https://godbolt.org/z/7cnWEMa6G

If we remove () here we should on line 17 in the previous file as well.

AlexGuteniev · 2022-01-07T14:45:01Z

stl/inc/vector

 }

+template <class _VbIt, class _OutIt>
+_CONSTEXPR20 _OutIt _Copy_vbool(_VbIt _First, _VbIt _Last, _OutIt _Dest) {


I'm questioning having this in the header, created #2462

I am wondering whether we should make them static methods inside of vector itself

I think method vs free function and in fully header / mostly in .cpp are orthogonal questions.
I have no opinion on method vs free function.

benchmarks/CMakeLists.txt

miscco · 2022-01-07T15:35:36Z

Can someone tell me whats wrong with the standard library header units?

miscco · 2022-01-07T15:40:51Z

Urgh looks like it is concerned about unreachable code because we always return for vbool

AlexGuteniev · 2022-01-07T19:19:49Z

A benchmark to consider to add when this is merged: #804 (comment)

jonwil · 2022-01-08T07:05:53Z

I didn't know 👻 was a valid type name in C++...
What is it actually supposed to be (its not clear from the PR)

SuperWig · 2022-01-08T08:08:15Z

It's the spooky std::vector<bool>

stl/inc/xutility

CaseyCarter

Partial review

README.md

CaseyCarter · 2022-02-16T20:46:30Z

README.md

+[documentation](https://github.com/google/benchmark/blob/main/docs/user_guide.md#running-a-subset-of-benchmarks))
+
+Right now the tests depend on an externally installed benchmark library. This can be done via `vcpkg install benchmark`.
+However, it has the drawback that it is not possible to benchmark code that links into the libraries provided by the


What "has the drawback"? Does "it is not possible to benchmark code that links into the libraries" mean that benchmarks can only use core headers? If so, we need to provide more explanation. If not, we need to clarify what it's trying to say.

I dropped that whole sentence because i actually never really tried

CaseyCarter · 2022-02-16T20:53:54Z

benchmarks/std/containers/sequences/vector.bool/copy_n/test.cpp

+    static random_device rd;
+    static mt19937 gen{random_device{}()};
+    vector<bool> result(size);
+    generate_n(result.begin(), size, []() { return bernoulli_distribution{0.5}(gen); });


If we remove () here we should on line 17 in the previous file as well.

benchmarks/std/containers/sequences/vector.bool/move/test.cpp

Co-authored-by: Casey Carter <[email protected]>

miscco · 2022-02-17T08:34:46Z

Note I left the whole vector bool tests disabled for EDG because it internally relies on vector which essentially breaks all the tests

StephanTLavavej · 2022-03-30T21:17:45Z

There's a merge conflict with main here.

strega-nil-ms · 2022-06-29T21:13:20Z

Assigning myself to help merge the benchmarks into the fancy benchmarking stuff added in #2780

strega-nil-ms · 2022-07-07T21:07:16Z

@miscco any interest in reviving this PR as of right now? Or should I mark draft?

miscco · 2022-07-08T05:11:40Z

There is definitely interest, but no time.

I just started a new job and we are moving later this month. So I would not expect any work this month

strega-nil-ms · 2022-07-08T17:29:22Z

Alright! I heard about your new job, good luck with that!!! I'm really happy for you 😁, and also good luck with moving!

I'll mark as draft for now; please ping me on the discord once you're ready to work on it again 😃

frederick-vs-ja · 2022-10-25T01:19:55Z

I wonder whether rotation (used in #3021) should be similarly optimized...

StephanTLavavej · 2023-01-04T22:27:04Z

We talked about this at the weekly maintainer meeting (first of the new year! 😸). While we remain interested in optimizing algorithms for vector<bool>, this specific PR is steadily becoming outdated with respect to the current state of main, and at this time neither @miscco nor the maintainers have capacity to bring this PR up to date. Therefore, we're going to close this without merging, but we will happily consider a new PR if anyone wants to restart this work. Thanks again @miscco for looking into this area! 😻

[vector] Add specialization of copy

3f2827e

miscco requested a review from a team as a code owner January 7, 2022 12:23

miscco changed the title ~~Vector bool copy~~ Optimize std::vector<:ghost:> copy algorithms Jan 7, 2022

miscco force-pushed the vector_bool_copy branch 2 times, most recently from a37cddd to 4376f0e Compare January 7, 2022 13:10

Add benchmarks to the STL

170e1ee

miscco force-pushed the vector_bool_copy branch from 4376f0e to 170e1ee Compare January 7, 2022 13:26

AlexGuteniev reviewed Jan 7, 2022

View reviewed changes

Fix snafu

5b6cd2b

miscco force-pushed the vector_bool_copy branch from 41ddca0 to 5b6cd2b Compare January 7, 2022 15:13

Review comments from Alex

56cd263

CaseyCarter added the performance Must go faster label Jan 7, 2022

miscco added 2 commits January 8, 2022 13:30

Work around unreachable code error

ce2d51d

More tests

26c0018

StephanTLavavej requested changes Feb 16, 2022

View reviewed changes

stl/inc/xutility Outdated Show resolved Hide resolved

miscco added 3 commits February 16, 2022 08:46

Merge branch 'main' into vector_bool_copy

b37dc82

Workaround VSO-1464637

a60a5a6

Be more fine grained with the tests

ee5e842

CaseyCarter reviewed Feb 16, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

StephanTLavavej closed this Feb 17, 2022

StephanTLavavej reopened this Feb 17, 2022

Apply Caseys review comments

49953e7

Co-authored-by: Casey Carter <[email protected]>

More review comments

916eeed

StephanTLavavej assigned mnatsuhara and StephanTLavavej and unassigned mnatsuhara Mar 9, 2022

StephanTLavavej assigned strega-nil-ms and unassigned StephanTLavavej Jun 29, 2022

strega-nil-ms marked this pull request as draft July 8, 2022 17:29

strega-nil-ms assigned miscco and unassigned strega-nil-ms Jul 20, 2022

StephanTLavavej unassigned miscco Aug 10, 2022

StephanTLavavej closed this Jan 4, 2023

StephanTLavavej mentioned this pull request Sep 21, 2023

[vector.bool] Optimize copy / move algorithms #3353

Merged

Optimize std::vector<:ghost:> copy algorithms #2461

Optimize std::vector<:ghost:> copy algorithms #2461

Uh oh!

Conversation

miscco commented Jan 7, 2022

Uh oh!

miscco commented Jan 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miscco commented Jan 7, 2022

Uh oh!

miscco commented Jan 7, 2022

Uh oh!

AlexGuteniev commented Jan 7, 2022

Uh oh!

jonwil commented Jan 8, 2022

Uh oh!

SuperWig commented Jan 8, 2022

Uh oh!

Uh oh!

CaseyCarter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

miscco commented Feb 17, 2022

Uh oh!

StephanTLavavej commented Mar 30, 2022

Uh oh!

strega-nil-ms commented Jun 29, 2022

Uh oh!

strega-nil-ms commented Jul 7, 2022

Uh oh!

miscco commented Jul 8, 2022

Uh oh!

strega-nil-ms commented Jul 8, 2022

Uh oh!

frederick-vs-ja commented Oct 25, 2022

Uh oh!

StephanTLavavej commented Jan 4, 2023

Uh oh!

Uh oh!