Before tackling bug 38722 make sure there is a baseline benchmark.
Details
- Reviewers
mclow.lists ldionne EricWF mvels - Group Reviewers
Restricted Project - Commits
- rG74a9c6d7e1c4: [libc++] Add a benchmark for std::map operations
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
There are several cases for "insert with hint". This tests one of them - the case where the hint is a dreferenceable iterator that points to the correct place in the map.
There are (at least) two other cases:
- The hint is a dreferenceable iterator that points to the wrong place in the map.
- The hint is not a dereferenceable iterator at all (think end())
I added 3 extra iterator positions for all hint cases:
- end() as you suggested.
- begin() this is most of the time the wrong iterator.
- the third element this is most of the time the wrong iterator. Just to avoid the code will use special optimizations on begin() and end().
Adds additional tests to makes sure the order doesn't result in always removing the first element as observed by @mclow.lists .
How did you compile that? ninja -C build cxx-benchmarks fail after applying your patch:
In file included from <llvm>/libcxx/benchmarks/map.bench.cpp:15: <llvm>/libcxx/benchmarks/CartesianBenchmarks.h:65:71: warning: pack fold expression is a C++17 extension [-Wc++17-extensions] (internal::makeBenchmarkImpl<B>(A, std::tuple<U..., T>(), rest...), ...); ^ <llvm>/libcxx/benchmarks/map.bench.cpp:77:9: error: cannot form a reference to 'void' auto& map = R.Maps.emplace_back(); ^ <llvm>/libcxx/benchmarks/map.bench.cpp:78:9: error: cannot form a reference to 'void' auto& hints = R.Hints.emplace_back(); ^ <llvm>/libcxx/benchmarks/map.bench.cpp:367:40: error: no member named 'insert_or_assign' in 'std::__1::map<unsigned long long, long long, std::__1::less<unsigned long long>, std::__1::allocator<std::__1::pair<const unsigned long long, long long> > >' benchmark::DoNotOptimize(Map.insert_or_assign(K, 1)); ~~~ ^ <llvm>/libcxx/benchmarks/CartesianBenchmarks.h:46:69: note: in instantiation of member function '(anonymous namespace)::InsertAssign<internal::EnumValue<(anonymous namespace)::AllModes, (anonymous namespace)::Mode, 1>, internal::EnumValue<(anonymous namespace)::AllOrders, (anonymous namespace)::Order, 1> >::run' requested here [=](benchmark::State& S) { Bench.run(S); }); ^ <llvm>/libcxx/benchmarks/CartesianBenchmarks.h:53:3: note: in instantiation of function template specialization 'internal::makeBenchmarkFromValuesImpl<(anonymous namespace)::InsertAssign<internal::EnumValue<(anonymous namespace)::AllModes, (anonymous namespace)::Mode, 1>, internal::EnumValue<(anonymous namespace)::AllOrders, (anonymous namespace)::Order, 1> >, std::__1::vector<std::__1::tuple<unsigned long>, std::__1::allocator<std::__1::tuple<unsigned long> > >, 0>' requested here makeBenchmarkFromValuesImpl<B>(A, std::index_sequence_for<Args...>()); ^ <llvm>/libcxx/benchmarks/CartesianBenchmarks.h:58:3: note: in instantiation of function template specialization 'internal::makeBenchmarkFromValues<(anonymous namespace)::InsertAssign<internal::EnumValue<(anonymous namespace)::AllModes, (anonymous namespace)::Mode, 1>, internal::EnumValue<(anonymous namespace)::AllOrders, (anonymous namespace)::Order, 1> >, unsigned long>' requested here makeBenchmarkFromValues<B<U...> >(A); ^
It builds for me, but I had CMAKE_CXX_STANDARD set to 17 for this build. It seems the method used to use C++17 for the benchmarks didn't work. I feel fixing this issue should not be part of this commit. So I created D75955 to fix the issue. With that patch applied it also works for me when CMAKE_CXX_STANDARD set to 14. This is the default value.
libcxx/benchmarks/CartesianBenchmarks.h | ||
---|---|---|
22 ↗ | (On Diff #249014) | Uh, EnumValue::value should already work, since it's inherited from integral constant, no? |
The fact that the keys you use to test are ordered is troubling to me. See comment on erase