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

Avoid unconditional make_heap for priority_queue::push_range #4025

Merged
merged 15 commits into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions benchmarks/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -108,5 +108,6 @@ endfunction()
add_benchmark(bitset_to_string src/bitset_to_string.cpp)
add_benchmark(locale_classic src/locale_classic.cpp)
add_benchmark(path_lexically_normal src/path_lexically_normal.cpp)
add_benchmark(priority_queue_push_range src/priority_queue_push_range.cpp)
add_benchmark(random_integer_generation src/random_integer_generation.cpp)
add_benchmark(std_copy src/std_copy.cpp)
87 changes: 87 additions & 0 deletions benchmarks/src/priority_queue_push_range.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Copyright (c) Microsoft Corporation.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#include <benchmark/benchmark.h>
#include <functional>
#include <queue>
#include <random>
#include <span>
#include <string>
#include <vector>
using namespace std;

namespace {
constexpr size_t vec_size = 10000;
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved

template <class T>
auto create_vec(size_t vsize, function<T(uint64_t)> transform) {
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
vector<T> vec(vsize);
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
for (mt19937_64 rnd(1); auto& e : vec) {
e = transform(rnd());
}
return vec;
}

template <class T>
T cast_to(uint64_t val) {
return static_cast<T>(val);
}

const auto vec_u8 = create_vec<uint8_t>(vec_size, cast_to<uint8_t>);
const auto vec_u16 = create_vec<uint16_t>(vec_size, cast_to<uint16_t>);
const auto vec_u32 = create_vec<uint32_t>(vec_size, cast_to<uint32_t>);
const auto vec_u64 = create_vec<uint64_t>(vec_size, cast_to<uint64_t>);
const auto vec_float = create_vec<float>(vec_size, cast_to<float>);
const auto vec_double = create_vec<double>(vec_size, cast_to<double>);

const auto vec_str =
create_vec<string>(vec_size, [](uint64_t val) { return to_string(static_cast<uint32_t>(val)); });
const auto vec_wstr =
create_vec<wstring>(vec_size, [](uint64_t val) { return to_wstring(static_cast<uint32_t>(val)); });
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved

template <class T, const auto& Data>
void BM_push_range(benchmark::State& state) {
const size_t frag_size = static_cast<size_t>(state.range(0));

for (auto _ : state) {
priority_queue<T> que;
span spn{Data};

while (!spn.empty()) {
size_t take_size = min(spn.size(), frag_size);
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
que.push_range(spn.subspan(0, take_size));
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
spn = spn.subspan(take_size);
}
benchmark::DoNotOptimize(que);
}
}

template <size_t L>
void putln(const benchmark::State&) {
static bool b = [] {
puts("");
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
return true;
}();
}
} // namespace

#define TEST_PUSH_RANGE(T, source) \
BENCHMARK(BM_push_range<T, source>) \
->Setup(putln<__LINE__>) \
->RangeMultiplier(100) \
->Range(1, vec_size) \
->Arg(vec_size / 2 + 1);
Copy link
Contributor Author

@achabense achabense Sep 13, 2023

Choose a reason for hiding this comment

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

For old version, vec_size/2+1 will make the function firstly make_heap the first half of data, then make_heap again after pushing the rest half of data:
image
For new version, vec_size/2+1 will make the function firstly make_heap the first half of data, then push_heap the rest half of data:
image


TEST_PUSH_RANGE(uint8_t, vec_u8);
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
TEST_PUSH_RANGE(uint16_t, vec_u16);
TEST_PUSH_RANGE(uint32_t, vec_u32);
TEST_PUSH_RANGE(uint64_t, vec_u64);
TEST_PUSH_RANGE(float, vec_float);
TEST_PUSH_RANGE(double, vec_double);

TEST_PUSH_RANGE(string_view, vec_str);
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
TEST_PUSH_RANGE(string, vec_str);
TEST_PUSH_RANGE(wstring_view, vec_wstr);
TEST_PUSH_RANGE(wstring, vec_wstr);

BENCHMARK_MAIN();
57 changes: 37 additions & 20 deletions stl/inc/queue
Original file line number Diff line number Diff line change
Expand Up @@ -246,40 +246,40 @@ public:
: c(), comp(_Pred) {}

priority_queue(const _Pr& _Pred, const _Container& _Cont) : c(_Cont), comp(_Pred) {
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}

priority_queue(const _Pr& _Pred, _Container&& _Cont) : c(_STD move(_Cont)), comp(_Pred) {
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}

template <class _InIt, enable_if_t<_Is_iterator_v<_InIt>, int> = 0>
priority_queue(_InIt _First, _InIt _Last, const _Pr& _Pred, const _Container& _Cont) : c(_Cont), comp(_Pred) {
c.insert(c.end(), _First, _Last);
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}

template <class _InIt, enable_if_t<_Is_iterator_v<_InIt>, int> = 0>
priority_queue(_InIt _First, _InIt _Last) : c(_First, _Last), comp() {
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}

template <class _InIt, enable_if_t<_Is_iterator_v<_InIt>, int> = 0>
priority_queue(_InIt _First, _InIt _Last, const _Pr& _Pred) : c(_First, _Last), comp(_Pred) {
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}

template <class _InIt, enable_if_t<_Is_iterator_v<_InIt>, int> = 0>
priority_queue(_InIt _First, _InIt _Last, const _Pr& _Pred, _Container&& _Cont) : c(_STD move(_Cont)), comp(_Pred) {
c.insert(c.end(), _First, _Last);
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}

#if _HAS_CXX23 && defined(__cpp_lib_concepts) // TRANSITION, GH-395
template <_Container_compatible_range<_Ty> _Rng>
priority_queue(from_range_t, _Rng&& _Range, const _Pr& _Pred = _Pr())
: c(_RANGES to<_Container>(_STD forward<_Rng>(_Range))), comp(_Pred) {
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}
#endif // _HAS_CXX23 && defined(__cpp_lib_concepts)

Expand All @@ -295,12 +295,12 @@ public:

template <class _Alloc, enable_if_t<uses_allocator_v<_Container, _Alloc>, int> = 0>
priority_queue(const _Pr& _Pred, const _Container& _Cont, const _Alloc& _Al) : c(_Cont, _Al), comp(_Pred) {
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}

template <class _Alloc, enable_if_t<uses_allocator_v<_Container, _Alloc>, int> = 0>
priority_queue(const _Pr& _Pred, _Container&& _Cont, const _Alloc& _Al) : c(_STD move(_Cont), _Al), comp(_Pred) {
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}

template <class _Alloc, enable_if_t<uses_allocator_v<_Container, _Alloc>, int> = 0>
Expand All @@ -315,45 +315,45 @@ public:
template <class _InIt, class _Alloc,
enable_if_t<_Is_iterator_v<_InIt> && uses_allocator_v<_Container, _Alloc>, int> = 0>
priority_queue(_InIt _First, _InIt _Last, const _Alloc& _Al) : c(_First, _Last, _Al), comp() {
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}

template <class _InIt, class _Alloc,
enable_if_t<_Is_iterator_v<_InIt> && uses_allocator_v<_Container, _Alloc>, int> = 0>
priority_queue(_InIt _First, _InIt _Last, const _Pr& _Pred, const _Alloc& _Al)
: c(_First, _Last, _Al), comp(_Pred) {
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}

template <class _InIt, class _Alloc,
enable_if_t<_Is_iterator_v<_InIt> && uses_allocator_v<_Container, _Alloc>, int> = 0>
priority_queue(_InIt _First, _InIt _Last, const _Pr& _Pred, const _Container& _Cont, const _Alloc& _Al)
: c(_Cont, _Al), comp(_Pred) {
c.insert(c.end(), _First, _Last);
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}

template <class _InIt, class _Alloc,
enable_if_t<_Is_iterator_v<_InIt> && uses_allocator_v<_Container, _Alloc>, int> = 0>
priority_queue(_InIt _First, _InIt _Last, const _Pr& _Pred, _Container&& _Cont, const _Alloc& _Al)
: c(_STD move(_Cont), _Al), comp(_Pred) {
c.insert(c.end(), _First, _Last);
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}

#if _HAS_CXX23 && defined(__cpp_lib_concepts) // TRANSITION, GH-395
template <_Container_compatible_range<_Ty> _Rng, class _Alloc,
enable_if_t<uses_allocator_v<_Container, _Alloc>, int> = 0>
priority_queue(from_range_t, _Rng&& _Range, const _Pr& _Pred, const _Alloc& _Al)
: c(_RANGES to<_Container>(_STD forward<_Rng>(_Range), _Al)), comp(_Pred) {
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}

template <_Container_compatible_range<_Ty> _Rng, class _Alloc,
enable_if_t<uses_allocator_v<_Container, _Alloc>, int> = 0>
priority_queue(from_range_t, _Rng&& _Range, const _Alloc& _Al)
: c(_RANGES to<_Container>(_STD forward<_Rng>(_Range), _Al)), comp() {
_STD make_heap(c.begin(), c.end(), comp);
_Make_heap();
}
#endif // _HAS_CXX23 && defined(__cpp_lib_concepts)

Expand All @@ -371,35 +371,47 @@ public:

void push(const value_type& _Val) {
c.push_back(_Val);
_STD push_heap(c.begin(), c.end(), comp);
_STD push_heap(c.begin(), c.end(), _STD _Pass_fn(comp));
}

void push(value_type&& _Val) {
c.push_back(_STD move(_Val));
_STD push_heap(c.begin(), c.end(), comp);
_STD push_heap(c.begin(), c.end(), _STD _Pass_fn(comp));
}

#if _HAS_CXX23 && defined(__cpp_lib_concepts) // TRANSITION, GH-395
template <_Container_compatible_range<_Ty> _Rng>
void push_range(_Rng&& _Range) {
const size_type _Old_size = c.size();

if constexpr (requires { c.append_range(_Range); }) {
c.append_range(_Range);
} else {
_RANGES copy(_Range, back_insert_iterator{c});
}

_STD make_heap(c.begin(), c.end(), comp);
const size_type _New_size = c.size();
if (_New_size / 2 > _Old_size) {
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
_Make_heap();
} else {
const auto _Begin = _STD _Get_unwrapped(c.begin());
auto _Heap_end = _Begin + _Old_size;
const auto _End = _STD _Get_unwrapped(c.end());
while (_Heap_end != _End) {
_STD push_heap(_Begin, ++_Heap_end, _STD _Pass_fn(comp));
}
}
}
#endif // _HAS_CXX23 && defined(__cpp_lib_concepts)

template <class... _Valty>
void emplace(_Valty&&... _Val) {
c.emplace_back(_STD forward<_Valty>(_Val)...);
_STD push_heap(c.begin(), c.end(), comp);
_STD push_heap(c.begin(), c.end(), _STD _Pass_fn(comp));
}

void pop() {
_STD pop_heap(c.begin(), c.end(), comp);
_STD pop_heap(c.begin(), c.end(), _STD _Pass_fn(comp));
c.pop_back();
}

Expand All @@ -410,6 +422,11 @@ public:
swap(comp, _Right.comp); // intentional ADL
}

private:
void _Make_heap() {
_STD make_heap(c.begin(), c.end(), _STD _Pass_fn(comp));
}

protected:
_Container c{};
_Pr comp{};
Expand Down
3 changes: 2 additions & 1 deletion stl/inc/xutility
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,8 @@ struct less_equal<void> {
template <class _Fx>
struct _Ref_fn { // pass function object by value as a reference
template <class... _Args>
constexpr decltype(auto) operator()(_Args&&... _Vals) { // forward function call operator
constexpr decltype(auto) operator()(_Args&&... _Vals) noexcept(
_Select_invoke_traits<_Fx&, _Args...>::_Is_nothrow_invocable::value) { // forward function call operator
Comment on lines -544 to +545
Copy link
Contributor

Choose a reason for hiding this comment

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

No change required - I don't have data to back up the claim - but there's a lot of expensive SFINAE in _Select_invoke_traits that _Ref_fn doesn't need. We may be better off with:

template <class _Fx, bool = is_member_pointer_v<_Fx>>
struct _Ref_fn { // pass function object by value as a reference
    template <class... _Args>
    constexpr decltype(auto) operator()(_Args&&... _Vals) noexcept(
        noexcept(_STD invoke(_Fn, _STD forward<_Args>(_Vals)...))) {
        return _STD invoke(_Fn, _STD forward<_Args>(_Vals)...);
    }

    _Fx& _Fn;
};

template <class _Fx>
struct _Ref_fn<_Fx, false> {
    template <class... _Args>
    constexpr decltype(auto) operator()(_Args&&... _Vals) noexcept(noexcept(_Fn(_STD forward<_Args>(_Vals)...))) {
        return _Fn(_STD forward<_Args>(_Vals)...);
    }

    _Fx& _Fn;
};

if constexpr (is_member_pointer_v<_Fx>) {
return _STD invoke(_Fn, _STD forward<_Args>(_Vals)...);
} else {
Expand Down