From d037848a96271873bd819b5e17af1ef5850dde08 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Fri, 18 Sep 2026 20:19:46 +0000 Subject: [PATCH] Replace hashtable ForEach callback with range-based iteration (#7806) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the callback-based `ForEach` methods on `RawHashtable`, `Map`, and `Set` with a range object supporting range-for loops, structured bindings, and the standard range concepts. - Adds `.entries()` on `Map`, `Set`, and `RawHashtable`, returning a range that models `std::ranges::forward_range` and `std::ranges::common_range`. Obtaining one is an explicit call rather than `begin()`/`end()` on the container, as scanning a whole table is costly and shouldn't be hidden. - Iterating a `Map` yields a `std::pair` of key and value references, which fits in two registers and is returned without being materialized in memory. - `Map::Range` and `Set::Range` are aliases of the raw hashtable's range rather than wrappers around it. The raw iterator produces the user-facing reference itself -- a `KeyT&` for a set, a pair of references for a map -- picked by `StorageEntry`, which is already specialized on whether there is a value type. That leaves one iterator to reason about instead of three. - Deletes the rvalue `.entries()` overloads on the owning containers, as a range built from a temporary table would dangle. Views don't own their storage, so the operation remains available on them. - In release builds, the walk over the groups is a single induction variable: a negative byte offset counting up to zero, anchored at the ends of the metadata and entry arrays. Both arrays are then reached by indexed addressing off a base that stays put, and the entry pointer is formed only once a group with a present entry has been found. - In debug builds, the range hashes the table's metadata when it is built and re-checks that hash when it is destroyed, catching mutation of the table while a range is live. It also picks a random starting group and a random odd group stride, which varies the traversal order between ranges while still visiting every group exactly once. That entropy is drawn when the range is built rather than in `begin()`, so `begin()` stays a pure function of the range and the multi-pass guarantee holds. - Removes `ForEachEntry` and all of its callers. Measured against the iteration benchmark added in its own commit, a traversal is at or ahead of what the callback compiled to across nearly the whole size range. The largest tables spend 3-5% fewer cycles, small `Set`s as much as 24% fewer, and instruction counts stay within about 1%. What remains behind is a handful of mid-sized `Map`s by up to 1%, and `Set` at 65536, which sits at exactly half its load factor, by 2%. Both revisions were built with `-c opt --copt=-march=x86-64-v3` and compared with: ``` ./scripts/bench_runner.py --exp_benchmark=... --base_benchmark=... \ --benchmark_args=--benchmark_perf_counters=INSTRUCTIONS,CYCLES \ --benchmark_args='--benchmark_filter=(Set|Map)Iterate<(Set|Map)<' \ --extra_metrics_filter='(INSTRUCTIONS|CYCLES)' ``` Trimmed below to the primary integer configurations and to the two counters; the pointer- and string-keyed configurations follow the same pattern. ``` Benchmark ┃ CYCLES ┃ INSTRUCTIONS ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BM_MapIterate>/1....... │ 👍 -6.032% p=1.14e-05 │ ?? p=0.752 baseline: │ 12.06 ± 1.520% │ 64 ± 3.125% experiment: │ 11.33 ± 2.765% │ 65.5 ± 3.817% │ │ BM_MapIterate>/2....... │ ?? p=0.155 │ ?? p=0.343 baseline: │ 7.587 ± 1.285% │ 41 ± 0.000% experiment: │ 7.652 ± 0.865% │ 41 ± 2.439% │ │ BM_MapIterate>/3....... │ ?? p=0.343 │ 👍 -1.020% p=0.0039 baseline: │ 6.663 ± 4.260% │ 32.67 ± 2.041% experiment: │ 6.368 ± 12.224% │ 32.33 ± 2.062% │ │ BM_MapIterate>/4....... │ ?? p=0.343 │ 👍 -1.786% p=0.0297 baseline: │ 6.091 ± 15.470% │ 28 ± 3.571% experiment: │ 5.957 ± 8.932% │ 27.5 ± 3.636% │ │ BM_MapIterate>/8....... │ ?? p=0.323 │ 👍 -1.220% p=0.000148 baseline: │ 4.845 ± 0.800% │ 20.5 ± 0.000% experiment: │ 4.814 ± 3.585% │ 20.25 ± 0.000% │ │ BM_MapIterate>/16...... │ 👍 -2.195% p=0.00908 │ 👍 0.769% p=6.58e-06 baseline: │ 4.312 ± 0.187% │ 16.25 ± 0.000% experiment: │ 4.218 ± 2.368% │ 16.13 ± 0.000% │ │ BM_MapIterate>/32...... │ ?? p=0.236 │ 👍 0.442% p=9.53e-06 baseline: │ 4.051 ± 1.084% │ 14.13 ± 0.000% experiment: │ 4.063 ± 0.737% │ 14.06 ± 0.000% │ │ BM_MapIterate>/64...... │ ?? p=0.693 │ 👎 0.227% p=4.52e-06 baseline: │ 4.021 ± 0.239% │ 13.75 ± 0.000% experiment: │ 4.019 ± 0.417% │ 13.78 ± 0.000% │ │ BM_MapIterate>/256..... │ 👍 0.360% p=0.00119 │ 👎 0.754% p=1.37e-05 baseline: │ 3.996 ± 0.173% │ 13.47 ± 0.000% experiment: │ 3.982 ± 0.272% │ 13.57 ± 0.000% │ │ BM_MapIterate>/4096.... │ 👍 0.581% p=1.96e-05 │ 👎 0.923% p=1.96e-05 baseline: │ 4.005 ± 0.816% │ 13.38 ± 0.000% experiment: │ 3.981 ± 0.192% │ 13.5 ± 0.000% │ │ BM_MapIterate>/65536... │ 👍 -4.957% p=1.14e-05 │ 👎 0.934% p=1.14e-05 baseline: │ 5.307 ± 0.501% │ 13.38 ± 0.000% experiment: │ 5.044 ± 1.746% │ 13.5 ± 0.000% │ │ BM_MapIterate>/1048576. │ 👍 -3.947% p=9.09e-05 │ 👎 0.935% p=3.3e-05 baseline: │ 6.074 ± 0.807% │ 13.38 ± 0.000% experiment: │ 5.834 ± 2.159% │ 13.5 ± 0.000% │ │ BM_MapIterate>/16777216 │ ?? p=0.155 │ 👎 0.935% p=2.11e-05 baseline: │ 5.082 ± 3.650% │ 13.38 ± 0.000% experiment: │ 5.012 ± 1.316% │ 13.5 ± 0.000% │ │ BM_MapIterate>/56...... │ 👎 0.825% p=0.0268 │ 👍 0.270% p=1.14e-05 baseline: │ 3.918 ± 0.501% │ 13.21 ± 0.000% experiment: │ 3.951 ± 0.342% │ 13.18 ± 0.000% │ │ BM_MapIterate>/224..... │ 👎 0.788% p=0.000504 │ 👎 0.346% p=1.64e-05 baseline: │ 3.895 ± 0.111% │ 12.89 ± 0.000% experiment: │ 3.926 ± 0.285% │ 12.94 ± 0.000% │ │ BM_MapIterate>/3584.... │ 👎 1.028% p=0.000148 │ 👎 0.545% p=1.14e-05 baseline: │ 3.913 ± 0.427% │ 12.79 ± 0.000% experiment: │ 3.954 ± 0.325% │ 12.86 ± 0.000% │ │ BM_MapIterate>/57344... │ ?? p=0.236 │ 👎 0.558% p=2.55e-06 baseline: │ 4.574 ± 0.721% │ 12.79 ± 0.000% experiment: │ 4.51 ± 3.709% │ 12.86 ± 0.000% │ │ BM_MapIterate>/917504.. │ 👍 -3.826% p=6.58e-06 │ 👎 0.559% p=2.33e-05 baseline: │ 5.221 ± 0.507% │ 12.79 ± 0.000% experiment: │ 5.021 ± 0.556% │ 12.86 ± 0.000% │ │ BM_MapIterate>/14680064 │ 👍 -3.839% p=1.37e-05 │ 👎 0.559% p=3.31e-05 baseline: │ 5.129 ± 1.194% │ 12.79 ± 0.000% experiment: │ 4.932 ± 1.475% │ 12.86 ± 0.000% │ │ Benchmark ┃ CYCLES ┃ INSTRUCTIONS ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BM_SetIterate>/1....... │ 👍 -3.104% p=0.000583 │ ?? p=0.206 baseline: │ 11.2 ± 6.323% │ 60 ± 3.333% experiment: │ 10.85 ± 5.820% │ 61 ± 3.279% │ │ BM_SetIterate>/2....... │ 👍 -7.037% p=0.0362 │ ?? p=0.155 baseline: │ 7.086 ± 16.857% │ 37 ± 0.000% experiment: │ 6.587 ± 0.479% │ 36 ± 4.167% │ │ BM_SetIterate>/3....... │ 👎 1.400% p=2.34e-05 │ 👍 -1.163% p=0.00136 baseline: │ 5.363 ± 0.463% │ 28.67 ± 2.326% experiment: │ 5.438 ± 32.763% │ 28.33 ± 1.176% │ │ BM_SetIterate>/4....... │ ?? p=0.968 │ ?? p=0.286 baseline: │ 4.642 ± 32.751% │ 23.5 ± 2.128% experiment: │ 4.658 ± 38.416% │ 23.63 ± 3.704% │ │ BM_SetIterate>/8....... │ 👍 -23.823% p=3.74e-06 │ 👍 -1.515% p=5.52e-05 baseline: │ 4.701 ± 6.589% │ 16.5 ± 0.000% experiment: │ 3.581 ± 7.790% │ 16.25 ± 0.000% │ │ BM_SetIterate>/16...... │ 👍 -4.502% p=1.37e-05 │ 👍 -1.020% p=3.31e-05 baseline: │ 3.124 ± 0.585% │ 12.25 ± 0.000% experiment: │ 2.983 ± 0.625% │ 12.13 ± 0.000% │ │ BM_SetIterate>/32...... │ 👍 -4.032% p=5.46e-06 │ 👍 0.617% p=1.96e-05 baseline: │ 2.957 ± 0.260% │ 10.13 ± 0.000% experiment: │ 2.838 ± 0.434% │ 10.06 ± 0.000% │ │ BM_SetIterate>/64...... │ 👍 -5.054% p=4.52e-06 │ 👎 0.321% p=1.37e-05 baseline: │ 2.937 ± 0.301% │ 9.75 ± 0.000% experiment: │ 2.788 ± 1.143% │ 9.781 ± 0.000% │ │ BM_SetIterate>/256..... │ 👍 -5.325% p=1.14e-05 │ 👎 1.073% p=6.58e-06 baseline: │ 2.916 ± 0.220% │ 9.469 ± 0.000% experiment: │ 2.761 ± 0.142% │ 9.57 ± 0.000% │ │ BM_SetIterate>/4096.... │ 👍 -4.865% p=4.52e-06 │ 👎 1.317% p=2.34e-05 baseline: │ 2.921 ± 0.194% │ 9.381 ± 0.000% experiment: │ 2.779 ± 0.224% │ 9.504 ± 0.000% │ │ BM_SetIterate>/65536... │ 👎 1.961% p=3.93e-05 │ 👎 1.332% p=1.49e-05 baseline: │ 4.015 ± 0.482% │ 9.375 ± 0.000% experiment: │ 4.094 ± 0.613% │ 9.5 ± 0.000% │ │ BM_SetIterate>/1048576. │ 👍 -4.843% p=1.14e-05 │ 👎 1.333% p=5.38e-06 baseline: │ 5.239 ± 0.144% │ 9.375 ± 0.000% experiment: │ 4.986 ± 0.139% │ 9.5 ± 0.000% │ │ BM_SetIterate>/16777216 │ 👍 0.840% p=0.0362 │ 👎 1.333% p=2.52e-06 baseline: │ 3.719 ± 1.420% │ 9.375 ± 0.000% experiment: │ 3.688 ± 1.308% │ 9.5 ± 0.000% │ │ BM_SetIterate>/56...... │ 👍 -2.857% p=9.53e-06 │ 👍 0.388% p=3.31e-05 baseline: │ 2.942 ± 0.439% │ 9.214 ± 0.000% experiment: │ 2.858 ± 0.619% │ 9.179 ± 0.000% │ │ BM_SetIterate>/224..... │ 👍 -2.161% p=2.34e-05 │ 👎 0.502% p=4.52e-06 baseline: │ 2.888 ± 0.347% │ 8.893 ± 0.000% experiment: │ 2.826 ± 0.450% │ 8.938 ± 0.000% │ │ BM_SetIterate>/3584.... │ 👍 -1.750% p=6.58e-06 │ 👎 0.793% p=2.34e-05 baseline: │ 2.89 ± 0.261% │ 8.792 ± 0.000% experiment: │ 2.84 ± 0.411% │ 8.862 ± 0.000% │ │ BM_SetIterate>/57344... │ ?? p=0.502 │ 👎 0.812% p=2.78e-05 baseline: │ 3.684 ± 4.246% │ 8.786 ± 0.000% experiment: │ 3.644 ± 4.431% │ 8.857 ± 0.000% │ │ BM_SetIterate>/917504.. │ 👍 -2.629% p=0.000148 │ 👎 0.813% p=3.08e-06 baseline: │ 4.372 ± 0.693% │ 8.786 ± 0.000% experiment: │ 4.257 ± 0.210% │ 8.857 ± 0.000% │ │ BM_SetIterate>/14680064 │ 👍 -2.927% p=0.0219 │ 👎 0.813% p=3.03e-06 baseline: │ 4.154 ± 3.286% │ 8.786 ± 0.000% experiment: │ 4.032 ± 3.198% │ 8.857 ± 0.000% │ │ ``` Assisted-by: Antigravity with Opus --- common/map.h | 39 +- common/map_benchmark.cpp | 58 +++ common/map_test.cpp | 115 +++++- common/raw_hashtable.cpp | 5 + common/raw_hashtable.h | 502 +++++++++++++++++++++---- common/set.h | 39 +- common/set_benchmark.cpp | 71 +++- common/set_test.cpp | 182 ++++++++- toolchain/benchmarking/source_gen.cpp | 15 +- toolchain/check/scope_stack.cpp | 8 +- toolchain/lower/specific_coalescer.cpp | 13 +- toolchain/testing/coverage_helper.h | 5 +- 12 files changed, 909 insertions(+), 143 deletions(-) diff --git a/common/map.h b/common/map.h index 5b5d4b0c23b6..82c7758ea998 100644 --- a/common/map.h +++ b/common/map.h @@ -69,6 +69,14 @@ class MapView using KeyContextT = ImplT::KeyContextT; using MetricsT = ImplT::MetricsT; + // A key and its value, as a pair of references. This is what iterating the + // map produces; there is no object in the table combining the two. + using Entry = ImplT::EntryRefT; + + // A range over the key-value entries of the map. Bound to the lifetime of + // the viewed map, and invalidated by mutating it. + using Range = ImplT::EntryRange; + // This type represents the result of lookup operations. It encodes whether // the lookup was a success as well as accessors for the key and value. class LookupKVResult { @@ -111,10 +119,8 @@ class MapView auto operator[](LookupKeyT lookup_key) const -> ValueT* requires(std::default_initializable); - // Run the provided callback for every key and value in the map. - template - auto ForEach(CallbackT callback) -> void - requires(std::invocable); + // Returns a range for iterating over all key-value entries in the map. + auto entries() const -> Range; // This routine is relatively inefficient and only intended for use in // benchmarking or logging of performance anomalies. The specific metrics @@ -169,6 +175,8 @@ class MapBase : protected RawHashtable::BaseImpl; using LookupKVResult = ViewT::LookupKVResult; using MetricsT = ImplT::MetricsT; + using Entry = ViewT::Entry; + using Range = ViewT::Range; // The result type for insertion operations both indicates whether an insert // was needed (as opposed to finding an existing element), and provides access @@ -228,12 +236,12 @@ class MapBase : protected RawHashtable::BaseImpl - auto ForEach(CallbackT callback) const -> void - requires(std::invocable) - { - return ViewT(*this).ForEach(callback); - } + auto entries() const& -> Range { return ViewT(*this).entries(); } + // Deleted on rvalues: the range refers to storage owned by this table, so a + // range built from a temporary map would dangle. Both qualifiers are needed + // as `&&` alone would leave a const rvalue binding to the `const&` overload. + auto entries() && = delete; + auto entries() const&& = delete; // Convenience forwarder to the view type. auto ComputeMetrics(KeyContextT key_context = KeyContextT()) const @@ -424,14 +432,9 @@ auto MapView::operator[]( } template -template -auto MapView::ForEach( - CallbackT callback) -> void - requires(std::invocable) -{ - this->ForEachEntry( - [callback](EntryT& entry) { callback(entry.key(), entry.value()); }, - [](auto...) {}); +auto MapView::entries() const + -> Range { + return this->ImplT::EntriesImpl(); } template diff --git a/common/map_benchmark.cpp b/common/map_benchmark.cpp index 1ece6d8405fe..8ee9f214f707 100644 --- a/common/map_benchmark.cpp +++ b/common/map_benchmark.cpp @@ -93,6 +93,17 @@ struct MapWrapperImpl { } auto BenchErase(KeyT k) -> bool { return m.erase(k) != 0; } + + // Visits every entry in the map, calling `cb` with the key and value of each + // one. Each map type is expected to traverse using whatever API it provides + // for this, so that the benchmark measures iterating the map rather than any + // specific iteration API. + template + auto BenchIterate(CallbackT cb) -> void { + for (const auto& entry : m) { + cb(entry.first, entry.second); + } + } }; // Explicit (partial) specialization for the Carbon map type that uses its @@ -126,6 +137,13 @@ struct MapWrapperImpl> { } auto BenchErase(KeyT k) -> bool { return m.Erase(k); } + + template + auto BenchIterate(CallbackT cb) -> void { + for (auto [k, v] : m.entries()) { + cb(k, v); + } + } }; // Provide a way to override the Carbon Map specific benchmark runs with another @@ -516,5 +534,45 @@ static void BM_MapInsertSeq(benchmark::State& state) { } MAP_BENCHMARK_ONE_OP(BM_MapInsertSeq, SizeArgs); +// Benchmark visiting every entry in a map. +// +// Unlike the lookup benchmarks, this walks the table's storage from end to end +// rather than probing it, so it is largely a measure of how densely entries are +// packed and how cheaply empty slots can be skipped. There is no dependency +// between the entries visited, and so this is a throughput measurement. +// +// Each batch is a single complete traversal of the map, with the batch size set +// to the number of entries so that the reported time is the per-entry cost. +template +static void BM_MapIterate(benchmark::State& state) { + using MapWrapperT = MapWrapper; + using KT = typename MapWrapperT::KeyT; + using VT = typename MapWrapperT::ValueT; + MapWrapperT m; + auto [keys, _] = GetKeysAndMissKeys(state.range(0)); + for (auto k : keys) { + bool inserted = m.BenchInsert(k, MakeValue()); + CARBON_DCHECK(inserted, "Must be a successful insert!"); + } + + while (state.KeepRunningBatch(keys.size())) { + ssize_t sum = 0; + m.BenchIterate([&sum](const KT& k, const VT& v) { + // Consume both the key and the value so that neither the traversal nor + // the loads out of the entries can be optimized away. + sum += ValueToBool(k) + ValueToBool(v); + }); + benchmark::DoNotOptimize(sum); + } + + // The time is already per-entry, so an iteration-invariant rate of one gives + // the throughput of entries visited. + state.counters["KeyRate"] = + benchmark::Counter(1, benchmark::Counter::kIsIterationInvariantRate); + + ReportMetrics(m, state); +} +MAP_BENCHMARK_ONE_OP(BM_MapIterate, SizeArgs); + } // namespace } // namespace Carbon diff --git a/common/map_test.cpp b/common/map_test.cpp index c2ede41181d1..c004a47bf901 100644 --- a/common/map_test.cpp +++ b/common/map_test.cpp @@ -7,7 +7,10 @@ #include #include +#include #include +#include +#include #include #include #include @@ -37,6 +40,7 @@ using RawHashtable::MoveOnlyTestData; using RawHashtable::TestData; using RawHashtable::TestKeyContext; using ::testing::Pair; +using ::testing::UnorderedElementsAre; using ::testing::UnorderedElementsAreArray; template @@ -47,9 +51,9 @@ auto ExpectMapElementsAre(MapT&& m, MatcherRangeT element_matchers) -> void { std::vector< std::pair, std::reference_wrapper>> map_entries; - m.ForEach([&map_entries](KeyT& k, ValueT& v) { + for (auto [k, v] : m.entries()) { map_entries.push_back({std::ref(k), std::ref(v)}); - }); + } // Use the GoogleMock unordered container matcher to validate and show errors // on wrong elements. @@ -865,5 +869,112 @@ TEST(MapContextTest, Basic) { m, MakeKeyValues([](int k) { return k * 100 + 1; }, llvm::seq(1, 512))); } +TYPED_TEST(MapTest, Range) { + using MapT = TypeParam; + using Range = decltype(std::declval().entries()); + using Iter = typename Range::Iterator; + + static_assert(std::forward_iterator); + static_assert(std::same_as().begin()), Iter>); + static_assert(std::same_as().end()), Iter>); + static_assert(std::ranges::forward_range); + static_assert(std::ranges::common_range); + + MapT m; + EXPECT_EQ(m.entries().begin(), m.entries().end()); + for (auto [k, v] : m.entries()) { + static_cast(k); + static_cast(v); + FAIL() << "Empty map range should have no elements"; + } + + for (int i = 1; i <= 5; ++i) { + m.Insert(i, i * 10); + } + + int count = 0; + for (const auto& [k, v] : m.entries()) { + EXPECT_EQ(v, m.Lookup(k).value()); + ++count; + } + EXPECT_EQ(count, 5); + + EXPECT_THAT(m.entries(), + UnorderedElementsAre(Pair(1, 10), Pair(2, 20), Pair(3, 30), + Pair(4, 40), Pair(5, 50))); + + using KeyT = typename MapT::KeyT; + using ValueT = typename MapT::ValueT; + using KeyContextT = typename MapT::KeyContextT; + MapView cv = m; + int cv_count = 0; + for (auto [k, v] : cv.entries()) { + static_assert(std::is_const_v>); + static_assert(std::is_const_v>); + EXPECT_EQ(v, m.Lookup(k).value()); + ++cv_count; + } + EXPECT_EQ(cv_count, 5); + EXPECT_THAT(cv.entries(), + UnorderedElementsAre(Pair(1, 10), Pair(2, 20), Pair(3, 30), + Pair(4, 40), Pair(5, 50))); + + for (auto [k, v] : m.entries()) { + if constexpr (requires { v.value; }) { + v.value = 99; + } else { + v = 99; + } + } + for (const auto& [k, v] : m.entries()) { + if constexpr (requires { v.value; }) { + EXPECT_EQ(v.value, 99); + } else { + EXPECT_EQ(v, 99); + } + } + EXPECT_THAT(m.entries(), + UnorderedElementsAre(Pair(1, 99), Pair(2, 99), Pair(3, 99), + Pair(4, 99), Pair(5, 99))); + + auto r = m.entries(); + int iter_count = 0; + for (auto it = r.begin(); it != r.end(); ++it) { + EXPECT_EQ(it->second, m.Lookup(it->first).value()); + EXPECT_EQ((*it).second, m.Lookup((*it).first).value()); + ++iter_count; + } + EXPECT_EQ(iter_count, 5); + + auto it = r.begin(); + auto prev = it++; + EXPECT_NE(it, prev); +} + +TYPED_TEST(MoveOnlyMapTest, Range) { + TypeParam m; + m.Insert(1, 10); + m.Insert(2, 20); + + int count = 0; + for (auto [k, v] : m.entries()) { + EXPECT_EQ(v.value, k.value * 10); + ++count; + } + EXPECT_EQ(count, 2); +} + +#ifndef NDEBUG +TEST(MapDeathTest, MutateDuringIterationFails) { + EXPECT_DEATH(([] { + Map m; + m.Insert(1, 10); + auto range = m.entries(); + m.Insert(2, 20); + }()), + "Hashtable mutated during iteration"); +} +#endif + } // namespace } // namespace Carbon::Testing diff --git a/common/raw_hashtable.cpp b/common/raw_hashtable.cpp index 25e2efd3e466..5a49dbb03610 100644 --- a/common/raw_hashtable.cpp +++ b/common/raw_hashtable.cpp @@ -10,4 +10,9 @@ namespace Carbon::RawHashtable { volatile std::byte global_addr_seed{1}; +#ifndef NDEBUG +std::atomic entropy_hash = + Carbon::HashValue(reinterpret_cast(&global_addr_seed)); +#endif + } // namespace Carbon::RawHashtable diff --git a/common/raw_hashtable.h b/common/raw_hashtable.h index 57d1be6dbd98..7650621bdba3 100644 --- a/common/raw_hashtable.h +++ b/common/raw_hashtable.h @@ -6,6 +6,7 @@ #define CARBON_COMMON_RAW_HASHTABLE_H_ #include +#include #include #include #include @@ -18,6 +19,7 @@ #include "common/concepts.h" #include "common/hashing.h" #include "common/raw_hashtable_metadata_group.h" +#include "llvm/ADT/iterator.h" #include "llvm/Support/Compiler.h" #include "llvm/Support/MathExtras.h" @@ -122,10 +124,15 @@ // null. Since it doesn't track the exact number of filled entries in a table, // it doesn't support a container-style `size` API. // -// - There is no direct iterator support because of the complexity of embedding -// the group-based metadata scanning into an iterator model. Instead, there is -// just a for-each method that is passed a lambda to observe all entries. The -// order of this observation is also not guaranteed. +// - Iteration is provided by a range object rather than by iterators hanging +// directly off the table, because the debug-only checks for mutation during +// iteration need state that outlives a single iterator: see `EntryRange` +// below. Obtaining one is an explicit call (`entries()`), as scanning an +// entire table is a costly operation that shouldn't be hidden behind a bare +// `begin()`/`end()` pair. +// +// The order of iteration is not guaranteed, and debug builds actively vary it +// between ranges to keep callers from depending on it. namespace Carbon::RawHashtable { // Which prefetch strategies to enable can be controlled via macros to enable @@ -152,7 +159,7 @@ inline constexpr ssize_t MinAllocatedSize = std::max(64, MaxGroupSize); // An entry in the hashtable storage of a `KeyT` and `ValueT` object. // // Allows manual construction, destruction, and access to these values so we can -// create arrays af the entries prior to populating them with actual keys and +// create arrays of the entries prior to populating them with actual keys and // values. template struct StorageEntry { @@ -168,6 +175,20 @@ struct StorageEntry { IsTriviallyRelocatable || (std::is_copy_constructible_v && std::is_copy_constructible_v); + // How iteration refers to an entry, and the iterator traits that follow. + // + // The key and value are stored side by side with nothing combining them, so + // a reference to an entry is a pair of references built on demand. That pair + // is a *proxy* reference: C++20 forward iterators permit one, but C++17 + // algorithms may assume a forward iterator's reference is a real lvalue, so + // the C++17 category is `input`. + using RefT = std::pair; + using IterValueT = RefT; + using IterPointerT = const RefT*; + using IterCategoryT = std::input_iterator_tag; + + auto ref() -> RefT { return RefT(key(), value()); } + auto key() const -> const KeyT& { // Ensure we don't need more alignment than available. Inside a method body // to apply to the complete type. @@ -194,11 +215,21 @@ struct StorageEntry { // construction. As a consequence, this struct only provides the storage and // we have to manually manage the construction, move, and destruction of the // objects. + // + // Destroys the key and value behind an entry reference. Iteration hands back + // `RefT` rather than the entry, so this is how a walked entry is destroyed. + static auto DestroyRef(RefT ref) -> void { + ref.first.~KeyT(); + ref.second.~ValueT(); + } + + // Destroys the key and value of this entry. The common case is destroying an + // entry found in the table's storage, where there is no reference to hand to + // `DestroyRef`. auto Destroy() -> void { static_assert(!IsTriviallyDestructible, "Should never instantiate when trivial!"); - key().~KeyT(); - value().~ValueT(); + DestroyRef(ref()); } auto CopyFrom(const StorageEntry& entry) -> void { @@ -241,6 +272,15 @@ struct StorageEntry { static constexpr bool IsCopyable = IsTriviallyRelocatable || std::is_copy_constructible_v; + // As above, but a set's entry is nothing but its key, so a reference to an + // entry is a true lvalue reference and the iterator is a plain forward one. + using RefT = KeyT&; + using IterValueT = std::remove_cv_t; + using IterPointerT = KeyT*; + using IterCategoryT = std::forward_iterator_tag; + + auto ref() -> RefT { return key(); } + auto key() const -> const KeyT& { // Ensure we don't need more alignment than available. static_assert( @@ -254,10 +294,12 @@ struct StorageEntry { return const_cast(const_cast(this)->key()); } + static auto DestroyRef(RefT ref) -> void { ref.~KeyT(); } + auto Destroy() -> void { static_assert(!IsTriviallyDestructible, "Should never instantiate when trivial!"); - key().~KeyT(); + DestroyRef(ref()); } auto CopyFrom(const StorageEntry& entry) -> void @@ -360,6 +402,13 @@ class ViewImpl { using EntryT = StorageEntry; using MetricsT = Metrics; + // What iterating over the table's entries produces: a `KeyT&` for a set, and + // a `std::pair` for a map. See `StorageEntry`. + using EntryRefT = EntryT::RefT; + + // The range type produced by `EntriesImpl`. + class EntryRange; + friend class BaseImpl; template friend class TableImpl; @@ -385,13 +434,11 @@ class ViewImpl { auto LookupEntry(LookupKeyT lookup_key, KeyContextT key_context) const -> EntryT*; - // Calls `entry_callback` for each entry in the hashtable. All the entries - // within a specific group are visited first, and then `group_callback` is - // called on the group itself. The `group_callback` is typically only used by - // the internals of the hashtable. - template - auto ForEachEntry(EntryCallbackT entry_callback, - GroupCallbackT group_callback) const -> void; + // Returns a range for iterating over all entries in the hashtable. + // + // The returned range copies this view, so it remains valid for as long as the + // underlying table does, independent of this view's lifetime. + auto EntriesImpl() const -> EntryRange; // Returns a collection of informative metrics on the the current state of the // table, useful for performance analysis. These include relatively slow to @@ -425,7 +472,7 @@ class ViewImpl { auto metadata() const -> uint8_t* { return reinterpret_cast(storage_); } - auto entries() const -> EntryT* { + auto entries_data() const -> EntryT* { return reinterpret_cast(reinterpret_cast(storage_) + EntriesOffset(alloc_size_)); } @@ -457,6 +504,172 @@ class ViewImpl { Storage* storage_; }; +// A range over the entries of a hashtable. +// +// A dedicated range object is used rather than a plain pair of iterators (such +// as `llvm::iterator_range`) because the range scopes two debug-only behaviors +// that a bare iterator pair has nowhere to store: +// +// - Mutation checking: the range snapshots a hash of the table's metadata on +// construction and re-checks it on destruction, catching tables that were +// mutated while iteration was active. +// - Traversal order: the group at which iteration starts, and the stride it +// walks the groups with, are drawn from an entropy pool once when the range +// is constructed. +// Deriving them here rather than in `begin()` keeps `begin()` a pure function +// of the range so that it can be called repeatedly, as forward ranges +// require, while still varying the order between separately created ranges. +// +// The range holds the view *by value*; views are two words and designed to be +// cheap to copy. It deliberately does not point back at the view it was created +// from, as views are routinely temporaries or by-value parameters whose +// lifetime is shorter than the table they refer to. +// +// This type provides only the minimal `begin()` and `end()` interface needed by +// range-based for loops and the range concepts, which also avoids any +// compile-time cost from including ``. +template +class ViewImpl::EntryRange { + public: + class Iterator; + + using value_type = typename EntryT::IterValueT; + using reference = EntryRefT; + using difference_type = ssize_t; + + explicit EntryRange(ViewImpl view); + + // Copyable: every member is a scalar snapshot of the table. Copying a range + // in a debug build simply validates the same table state more than once. + EntryRange(const EntryRange&) = default; + auto operator=(const EntryRange&) -> EntryRange& = default; + +#ifndef NDEBUG + // Only debug builds declare a destructor, and so only they re-check the + // table on the way out. Release builds leave the range trivially + // destructible, and so trivial for the purposes of calls, letting it be + // passed and returned in registers. + ~EntryRange() { CheckInvariants(); } +#endif + + auto begin() const -> Iterator; + auto end() const -> Iterator; + + private: + // The facade `Iterator` derives from. A class can't name one of its own + // aliases in its base-specifier, so naming it here lets `Iterator` spell it + // once instead of repeating it to get at the members it inherits. + using IteratorBase = + llvm::iterator_facade_base; + +#ifndef NDEBUG + // Checks that the table's metadata has not changed since construction. + auto CheckInvariants() const -> void; +#endif + + ViewImpl view_; +#ifndef NDEBUG + HashCode initial_metadata_hash_ = {}; + ssize_t start_group_ = 0; + ssize_t step_ = GroupSize; +#endif +}; + +// Two-level forward iterator through present hashtable entries. +template +class ViewImpl::EntryRange::Iterator + : public EntryRange::IteratorBase { + public: + // Both the set and map forms satisfy C++20's `std::forward_iterator`. A + // map's `reference` is a proxy, which pins its C++17 `iterator_category` to + // `input`, but the C++20 concept is unaffected. See `EntryRefT`. + using iterator_concept = std::forward_iterator_tag; + + Iterator() = default; + + using EntryRange::IteratorBase::operator++; + + [[clang::always_inline]] auto operator*() const -> EntryRefT { + CARBON_DCHECK(present_bits_ != 0, "Dereferencing end iterator!"); + __builtin_assume(present_bits_ != 0); + // `index_ptr` folds scaling the match index by the entry size together + // with decoding the index itself, which saves a shift on the portable + // byte-encoded code path. + return MatchIndex(present_bits_).index_ptr(group_entries())->ref(); + } + + [[clang::always_inline]] auto operator++() -> Iterator& { + CARBON_DCHECK(present_bits_ != 0, "Incrementing end iterator!"); + __builtin_assume(present_bits_ != 0); + present_bits_ &= (present_bits_ - 1); + if (LLVM_LIKELY(present_bits_ != 0)) { + return *this; + } + AdvanceToNextPresentGroup(); + return *this; + } + + friend auto operator==(const Iterator& lhs, const Iterator& rhs) -> bool { + if (lhs.present_bits_ == 0 || rhs.present_bits_ == 0) { + return lhs.present_bits_ == rhs.present_bits_; + } + // The entry pointer already encodes the base and the group offset, so it + // uniquely identifies the group without a separate index. + return lhs.group_entries() == rhs.group_entries() && + lhs.present_bits_ == rhs.present_bits_; + } + + private: + friend class EntryRange; + + using MatchBitsT = typename MetadataGroup::MatchPresentRange::BitsT; + using MatchIndex = typename MetadataGroup::MatchIndex; + + // Builds an iterator to the first present entry of `range`, or an iterator + // equal to `end()` when the range has no entries to walk. The parameters of + // the walk differ between builds, so both are drawn from the range here + // rather than passed in. + [[clang::always_inline]] explicit Iterator(const EntryRange& range); + + [[clang::always_inline]] auto AdvanceToNextPresentGroup() -> void; + + // The entries of the group the iterator is currently within. Both builds + // track the current group, but they encode it differently, so the encoding + // is hidden behind this accessor. + auto group_entries() const -> EntryT* { +#ifndef NDEBUG + return group_entries_; +#else + return entries_end_ + group_offset_; +#endif + } + +#ifndef NDEBUG + // Debug builds walk groups in a randomized order and so must retain the + // array bases along with the parameters of the walk. The randomized walk + // revisits no group but also never reaches the end of the array, so it does + // need an explicit count of the groups left to visit. + EntryT* group_entries_ = nullptr; + const uint8_t* metadata_ = nullptr; + EntryT* entries_ = nullptr; + ssize_t groups_remaining_ = 0; + ssize_t group_index_ = 0; + size_t probe_mask_ = 0; + ssize_t step_ = GroupSize; +#else + // Release builds walk the groups in order, tracking the position as a + // *negative* byte offset from the end of each array that counts up to zero. + // Anchoring at the ends rather than the beginnings means the walk needs only + // this one induction variable, and reaching zero is the bound. + EntryT* entries_end_ = nullptr; + const uint8_t* metadata_end_ = nullptr; + ssize_t group_offset_ = 0; +#endif + MatchBitsT present_bits_ = 0; +}; + // Implementation helper for defining a read-write base type for a hashtable // that type-erases any SSO buffer. // @@ -495,7 +708,10 @@ class BaseImpl { // NOLINTNEXTLINE(google-explicit-constructor): Designed to implicitly decay. explicit(false) operator ViewImplT() const { return view_impl(); } - auto view_impl() const -> ViewImplT { return view_impl_; } + auto view_impl() const -> const ViewImplT& { return view_impl_; } + + // Destroys all non-trivially destructible entries in the table. + auto DestroyEntries() -> void; // Looks up the provided key in the hashtable. If found, returns a pointer to // that entry and `false`. @@ -510,7 +726,7 @@ class BaseImpl { // Grow the table to specific allocation size. // - // This will grow the the table if necessary for it to have an allocation size + // This will grow the table if necessary for it to have an allocation size // of `target_alloc_size` which must be a power of two. Note that this will // not allow that many keys to be inserted into the hashtable, but a smaller // number based on the load factor. If a specific number of insertions need to @@ -561,7 +777,7 @@ class BaseImpl { auto storage() const -> Storage* { return view_impl_.storage_; } auto storage() -> Storage*& { return view_impl_.storage_; } auto metadata() const -> uint8_t* { return view_impl_.metadata(); } - auto entries() const -> EntryT* { return view_impl_.entries(); } + auto entries_data() const -> EntryT* { return view_impl_.entries_data(); } auto small_alloc_size() const -> ssize_t { return static_cast(small_alloc_size_); } @@ -665,6 +881,25 @@ inline auto ComputeSeed() -> uint64_t { return reinterpret_cast(&global_addr_seed); } +#ifndef NDEBUG +// A pool of entropy used to vary the iteration order of hashtables in debug +// builds. It is seeded from ASLR where available. +extern std::atomic entropy_hash; + +// Returns a pseudo-random value from the entropy pool, advancing the pool. +// +// The load and store are separate relaxed operations rather than one atomic +// read-modify-write so that consuming entropy is just a load, and refreshing +// the pool doesn't block the iteration that follows. Racing callers can lose an +// update and draw the same value, which is fine for a debug aid. +inline auto NextRangeEntropy() -> HashCode { + HashCode prev_entropy_hash = entropy_hash.load(std::memory_order_relaxed); + entropy_hash.store(Carbon::HashValue(prev_entropy_hash), + std::memory_order_relaxed); + return prev_entropy_hash; +} +#endif + inline auto ComputeProbeMaskFromSize(ssize_t size) -> size_t { CARBON_DCHECK(llvm::isPowerOf2_64(size), "Size must be a power of two for a hashed buffer!"); @@ -748,7 +983,7 @@ auto ViewImpl::LookupEntry( HashCode hash = key_context.HashKey(lookup_key, ComputeSeed()); auto [hash_index, tag] = hash.ExtractIndexAndTag<7>(); - EntryT* local_entries = entries(); + EntryT* local_entries = entries_data(); // Walk through groups of entries using a quadratic probe starting from // `hash_index`. @@ -799,41 +1034,11 @@ auto ViewImpl::LookupEntry( } while (LLVM_UNLIKELY(true)); } -// Note that we force inlining here because we expect to be called with lambdas -// that will in turn be inlined to form the loop body. We don't want function -// boundaries within the loop for performance, and recognizing the degree of -// simplification from inlining these callbacks may be difficult to -// automatically recognize. -template -template -[[clang::always_inline]] auto -ViewImpl::ForEachEntry( - EntryCallbackT entry_callback, GroupCallbackT group_callback) const - -> void { - uint8_t* local_metadata = metadata(); - EntryT* local_entries = entries(); - - ssize_t local_size = alloc_size_; - for (ssize_t group_index = 0; group_index < local_size; - group_index += GroupSize) { - auto g = MetadataGroup::Load(local_metadata, group_index); - auto present_matched_range = g.MatchPresent(); - if (!present_matched_range) { - continue; - } - for (ssize_t byte_index : present_matched_range) { - entry_callback(local_entries[group_index + byte_index]); - } - - group_callback(&local_metadata[group_index]); - } -} - template auto ViewImpl::ComputeMetricsImpl( KeyContextT key_context) const -> Metrics { uint8_t* local_metadata = metadata(); - EntryT* local_entries = entries(); + EntryT* local_entries = entries_data(); ssize_t local_size = alloc_size_; Metrics metrics; @@ -898,6 +1103,147 @@ auto ViewImpl::ComputeMetricsImpl( return metrics; } +template +[[clang::always_inline]] auto +ViewImpl::EntriesImpl() const + -> EntryRange { + return EntryRange(*this); +} + +template +[[clang::always_inline]] +ViewImpl::EntryRange::Iterator:: + Iterator(const EntryRange& range) { + const ViewImpl& view = range.view_; + ssize_t alloc_size = view.alloc_size_; + + // An empty or moved-from table has no groups to load from, and the + // default-initialized state left behind already compares equal to `end()`. + if (alloc_size == 0 || view.storage_ == nullptr) { + return; + } + +#ifndef NDEBUG + entries_ = view.entries_data(); + metadata_ = view.metadata(); + // The starting group and stride were drawn when the range was constructed, + // so every iterator built from it walks the same order. + group_index_ = range.start_group_; + group_entries_ = entries_ + group_index_; + groups_remaining_ = alloc_size / GroupSize - 1; + probe_mask_ = ComputeProbeMaskFromSize(alloc_size); + step_ = range.step_; + + auto g = MetadataGroup::Load(metadata_, group_index_); +#else + // The allocation size bounds the metadata array directly, so anchoring at + // the ends of the arrays lets the walk run off a single induction variable + // without ever dividing by the group size. + entries_end_ = view.entries_data() + alloc_size; + metadata_end_ = view.metadata() + alloc_size; + group_offset_ = -alloc_size; + + auto g = MetadataGroup::Load(metadata_end_, group_offset_); +#endif + + auto present_range = g.MatchPresent(); + if (present_range) { + present_bits_ = static_cast(present_range); + } else { + AdvanceToNextPresentGroup(); + } +} + +template +[[clang::always_inline]] auto +ViewImpl::EntryRange::Iterator::AdvanceToNextPresentGroup() + -> void { +#ifndef NDEBUG + while (--groups_remaining_ >= 0) { + group_index_ = static_cast( + static_cast(group_index_ + step_) & probe_mask_); + auto g = MetadataGroup::Load(metadata_, group_index_); + auto range = g.MatchPresent(); + if (range) { + group_entries_ = entries_ + group_index_; + present_bits_ = static_cast(range); + return; + } + } +#else + for (group_offset_ += GroupSize; group_offset_ != 0; + group_offset_ += GroupSize) { + auto g = MetadataGroup::Load(metadata_end_, group_offset_); + auto range = g.MatchPresent(); + if (range) { + present_bits_ = static_cast(range); + return; + } + } +#endif + present_bits_ = 0; +} + +template +ViewImpl::EntryRange::EntryRange( + ViewImpl view) + : view_(view) { +#ifndef NDEBUG + if (view_.alloc_size_ <= 0 || view_.storage_ == nullptr) { + return; + } + initial_metadata_hash_ = Carbon::HashValue( + llvm::ArrayRef(view_.metadata(), view_.alloc_size_)); + + // Draw the traversal order once, here, so that `begin()` remains a pure + // function of the range and can be called repeatedly. Two separately + // constructed ranges still walk the table in different orders. + start_group_ = NextRangeEntropy().ExtractIndex() & + ComputeProbeMaskFromSize(view_.alloc_size_); + + // Walk the groups with a stride of an odd number of groups. The group count + // is always a power of two, so any odd stride is coprime with it and visits + // every group exactly once before repeating. That scrambles the group order + // far more thoroughly than a forward or reverse scan, and costs nothing in + // the loop itself as the increment already adds a stride and masks. + ssize_t num_groups = view_.alloc_size_ / GroupSize; + ssize_t stride_groups = + (NextRangeEntropy().ExtractIndex() & (num_groups - 1)) | 1; + step_ = stride_groups * GroupSize; +#endif +} + +#ifndef NDEBUG +template +auto ViewImpl::EntryRange::CheckInvariants() const -> void { + if (view_.alloc_size_ <= 0 || view_.storage_ == nullptr) { + return; + } + HashCode current_hash = Carbon::HashValue( + llvm::ArrayRef(view_.metadata(), view_.alloc_size_)); + CARBON_CHECK(current_hash == initial_metadata_hash_, + "Hashtable mutated during iteration: metadata changed!"); +} +#endif + +template +[[clang::always_inline]] auto +ViewImpl::EntryRange::begin() const + -> Iterator { + // The traversal order is fixed when the range is constructed, so repeated + // calls yield equal iterators as forward ranges require. + return Iterator(*this); +} + +template +[[clang::always_inline]] auto +ViewImpl::EntryRange::end() const + -> Iterator { + return Iterator(); +} + // TODO: Evaluate whether it is worth forcing this out-of-line given the // reasonable ABI boundary it forms and large volume of code necessary to // implement it. @@ -921,7 +1267,7 @@ auto BaseImpl::InsertImpl( ssize_t group_with_deleted_index; MetadataGroup::MatchIndex deleted_match = {}; - EntryT* local_entries = entries(); + EntryT* local_entries = entries_data(); auto return_insert_at_index = [&](ssize_t index) -> std::pair { // We'll need to insert at this index so set the control group byte to the @@ -1017,7 +1363,7 @@ BaseImpl::GrowToAllocSizeImpl( bool old_small = is_small(); Storage* old_storage = storage(); uint8_t* old_metadata = metadata(); - EntryT* old_entries = entries(); + EntryT* old_entries = entries_data(); // Configure for the new size and allocate the new storage. alloc_size() = target_alloc_size; @@ -1093,7 +1439,7 @@ auto BaseImpl::EraseImpl( // If we mark the slot as empty, we'll also need to increase the growth // budget. uint8_t* local_metadata = metadata(); - EntryT* local_entries = entries(); + EntryT* local_entries = entries_data(); ssize_t index = entry - local_entries; ssize_t group_index = index & ~GroupMask; auto g = MetadataGroup::Load(local_metadata, group_index); @@ -1114,16 +1460,10 @@ auto BaseImpl::EraseImpl( template auto BaseImpl::ClearImpl() -> void { - view_impl_.ForEachEntry( - [](EntryT& entry) { - if constexpr (!EntryT::IsTriviallyDestructible) { - entry.Destroy(); - } - }, - [](uint8_t* metadata_group) { - // Clear the group. - std::memset(metadata_group, 0, GroupSize); - }); + DestroyEntries(); + if (storage() != nullptr) { + std::memset(metadata(), 0, alloc_size()); + } growth_budget_ = GrowthThresholdForAllocSize(alloc_size()); } @@ -1186,10 +1526,7 @@ auto BaseImpl::Destroy() -> void { } // Destroy all the entries. - if constexpr (!EntryT::IsTriviallyDestructible) { - view_impl_.ForEachEntry([](EntryT& entry) { entry.Destroy(); }, - [](auto...) {}); - } + DestroyEntries(); // If small, nothing to deallocate. if (is_small()) { @@ -1201,6 +1538,16 @@ auto BaseImpl::Destroy() -> void { Deallocate(storage(), alloc_size()); } +template +auto BaseImpl::DestroyEntries() + -> void { + if constexpr (!EntryT::IsTriviallyDestructible) { + for (typename EntryT::RefT entry : view_impl_.EntriesImpl()) { + EntryT::DestroyRef(entry); + } + } +} + // Copy all of the slots over from another table that is exactly the same // allocation size. // @@ -1224,9 +1571,9 @@ auto BaseImpl::CopySlotsFrom( // all of the keys. This is especially important as we don't have an easy way // to access the key context needed for rehashing here. uint8_t* local_metadata = metadata(); - EntryT* local_entries = entries(); + EntryT* local_entries = entries_data(); const uint8_t* local_arg_metadata = arg.metadata(); - const EntryT* local_arg_entries = arg.entries(); + const EntryT* local_arg_entries = arg.entries_data(); memcpy(local_metadata, local_arg_metadata, local_size); for (ssize_t group_index = 0; group_index < local_size; @@ -1269,9 +1616,9 @@ auto BaseImpl::MoveFrom( // themselves. We do this preserving their slots and even tombstones to // avoid rehashing. uint8_t* local_metadata = this->metadata(); - EntryT* local_entries = this->entries(); + EntryT* local_entries = this->entries_data(); uint8_t* local_arg_metadata = arg.metadata(); - EntryT* local_arg_entries = arg.entries(); + EntryT* local_arg_entries = arg.entries_data(); memcpy(local_metadata, local_arg_metadata, local_size); if (EntryT::IsTriviallyRelocatable) { memcpy(local_entries, local_arg_entries, local_size * sizeof(EntryT)); @@ -1306,7 +1653,7 @@ auto BaseImpl::InsertIntoEmpty( HashCode hash) -> EntryT* { auto [hash_index, tag] = hash.ExtractIndexAndTag<7>(); uint8_t* local_metadata = metadata(); - EntryT* local_entries = entries(); + EntryT* local_entries = entries_data(); for (ProbeSequence s(hash_index, alloc_size());; s.Next()) { ssize_t group_index = s.index(); @@ -1392,7 +1739,7 @@ auto BaseImpl::GrowToNextAllocSize( bool old_small = is_small(); Storage* old_storage = storage(); uint8_t* old_metadata = metadata(); - EntryT* old_entries = entries(); + EntryT* old_entries = entries_data(); #ifndef NDEBUG // Count how many of the old table slots will end up being empty after we grow @@ -1417,7 +1764,7 @@ auto BaseImpl::GrowToNextAllocSize( // Now extract the new components of the table. uint8_t* new_metadata = metadata(); - EntryT* new_entries = entries(); + EntryT* new_entries = entries_data(); // Walk the metadata groups, clearing deleted to empty, duplicating the // metadata for the low and high halves, and updating it based on where each @@ -1596,10 +1943,7 @@ auto TableImpl::operator=(const TableImpl& arg) return *this; } CARBON_DCHECK(arg.storage() != this->storage()); - if constexpr (!EntryT::IsTriviallyDestructible) { - this->view_impl_.ForEachEntry([](EntryT& entry) { entry.Destroy(); }, - [](auto...) {}); - } + this->DestroyEntries(); } else { // The sizes don't match so destroy everything and re-setup the table // storage. diff --git a/common/set.h b/common/set.h index 7d1c3d8b42eb..f38c7bfd709d 100644 --- a/common/set.h +++ b/common/set.h @@ -7,6 +7,7 @@ #include #include +#include #include "common/check.h" #include "common/hashtable_key_context.h" @@ -60,6 +61,10 @@ class SetView : RawHashtable::ViewImpl { using KeyContextT = ImplT::KeyContextT; using MetricsT = ImplT::MetricsT; + // A range over the keys of the set. Bound to the lifetime of the viewed set, + // and invalidated by mutating it. + using Range = ImplT::EntryRange; + // This type represents the result of lookup operations. It encodes whether // the lookup was a success as well as accessors for the key. class LookupResult { @@ -91,10 +96,8 @@ class SetView : RawHashtable::ViewImpl { auto Lookup(LookupKeyT lookup_key, KeyContextT key_context = KeyContextT()) const -> LookupResult; - // Run the provided callback for every key in the set. - template - auto ForEach(CallbackT callback) const -> void - requires(std::invocable); + // Returns a range for iterating over all keys in the set. + auto entries() const -> Range; // This routine is relatively inefficient and only intended for use in // benchmarking or logging of performance anomalies. The specific metrics @@ -143,6 +146,7 @@ class SetBase using ViewT = SetView; using LookupResult = ViewT::LookupResult; using MetricsT = ImplT::MetricsT; + using Range = ViewT::Range; // The result type for insertion operations both indicates whether an insert // was needed (as opposed to the key already being in the set), and provides @@ -190,12 +194,12 @@ class SetBase } // Convenience forwarder to the view type. - template - auto ForEach(CallbackT callback) const -> void - requires(std::invocable) - { - return ViewT(*this).ForEach(callback); - } + auto entries() const& -> Range { return ViewT(*this).entries(); } + // Deleted on rvalues: the range refers to storage owned by this table, so a + // range built from a temporary set would dangle. Both qualifiers are needed + // as `&&` alone would leave a const rvalue binding to the `const&` overload. + auto entries() && = delete; + auto entries() const&& = delete; // Convenience forwarder to the view type. auto ComputeMetrics(KeyContextT key_context = KeyContextT()) const @@ -211,10 +215,10 @@ class SetBase auto Insert(LookupKeyT lookup_key, KeyContextT key_context = KeyContextT()) -> InsertResult; - // Insert a key into the map and call the provided callback if necessary to - // produce a new key when no existing value is found. + // Insert a key into the set and call the provided callback if necessary to + // produce a new key when no existing key is found. // - // Example: `m.Insert(key_equivalent, [] { return real_key; });` + // Example: `s.Insert(key_equivalent, [] { return real_key; });` // // The point of this function is when the lookup key is _different_from the // stored key. However, we don't restrict it in case that blocks generic @@ -333,13 +337,8 @@ auto SetView::Lookup(LookupKeyT lookup_key, } template -template -auto SetView::ForEach(CallbackT callback) const - -> void - requires(std::invocable) -{ - this->ForEachEntry([callback](EntryT& entry) { callback(entry.key()); }, - [](auto...) {}); +auto SetView::entries() const -> Range { + return this->ImplT::EntriesImpl(); } template diff --git a/common/set_benchmark.cpp b/common/set_benchmark.cpp index 6c5dc6d809a0..111a6e582687 100644 --- a/common/set_benchmark.cpp +++ b/common/set_benchmark.cpp @@ -35,8 +35,9 @@ static constexpr bool IsCarbonSet = IsCarbonSetImpl::value; // support different APIs. The primary template assumes a roughly // `std::unordered_set` API design, and types with a different API design are // supported through specializations. -template +template struct SetWrapperImpl { + using SetT = InSetT; using KeyT = SetT::key_type; SetT s; @@ -58,6 +59,17 @@ struct SetWrapperImpl { } auto BenchErase(KeyT k) -> bool { return s.erase(k) != 0; } + + // Visits every key in the set, calling `cb` with each one. Each set type is + // expected to traverse using whatever API it provides for this, so that the + // benchmark measures iterating the set rather than any specific iteration + // API. + template + auto BenchIterate(CallbackT cb) -> void { + for (const auto& k : s) { + cb(k); + } + } }; // Explicit (partial) specialization for the Carbon map type that uses its @@ -85,6 +97,13 @@ struct SetWrapperImpl> { } auto BenchErase(KeyT k) -> bool { return s.Erase(k); } + + template + auto BenchIterate(CallbackT cb) -> void { + for (const auto& k : s.entries()) { + cb(k); + } + } }; // Provide a way to override the Carbon Set specific benchmark runs with another @@ -123,6 +142,17 @@ using SetWrapper = SetWrapperOverride; #endif +// Reports extra statistics about the table, when it is in fact a Carbon table. +// Note that this has to inspect the *wrapped* type in order to work correctly +// when the Carbon benchmarks are overridden with another implementation. +template +auto ReportMetrics(const SetWrapper& s_wrapper, benchmark::State& state) + -> void { + if constexpr (IsCarbonSet::SetT>) { + ReportTableMetrics(s_wrapper.s, state); + } +} + // NOLINTBEGIN(bugprone-macro-parentheses): Parentheses are incorrect here. #define MAP_BENCHMARK_ONE_OP_SIZE(NAME, APPLY, KT) \ BENCHMARK(NAME>)->Apply(APPLY); \ @@ -375,5 +405,44 @@ static void BM_SetInsertSeq(benchmark::State& state) { } MAP_BENCHMARK_OP_SEQ(BM_SetInsertSeq); +// Benchmark visiting every key in a set. +// +// Unlike the lookup benchmarks, this walks the table's storage from end to end +// rather than probing it, so it is largely a measure of how densely keys are +// packed and how cheaply empty slots can be skipped. There is no dependency +// between the keys visited, and so this is a throughput measurement. +// +// Each batch is a single complete traversal of the set, with the batch size set +// to the number of keys so that the reported time is the per-key cost. +template +static void BM_SetIterate(benchmark::State& state) { + using SetWrapperT = SetWrapper; + using KT = typename SetWrapperT::KeyT; + SetWrapperT s; + auto [keys, _] = GetKeysAndMissKeys(state.range(0)); + for (auto k : keys) { + bool inserted = s.BenchInsert(k); + CARBON_DCHECK(inserted, "Must be a successful insert!"); + } + + while (state.KeepRunningBatch(keys.size())) { + ssize_t sum = 0; + s.BenchIterate([&sum](const KT& k) { + // Consume the key so that neither the traversal nor the loads out of the + // entries can be optimized away. + sum += ValueToBool(k); + }); + benchmark::DoNotOptimize(sum); + } + + // The time is already per-key, so an iteration-invariant rate of one gives + // the throughput of keys visited. + state.counters["KeyRate"] = + benchmark::Counter(1, benchmark::Counter::kIsIterationInvariantRate); + + ReportMetrics(s, state); +} +MAP_BENCHMARK_ONE_OP(BM_SetIterate, SizeArgs); + } // namespace } // namespace Carbon diff --git a/common/set_test.cpp b/common/set_test.cpp index bf8b8fa279a1..68fc788ef65f 100644 --- a/common/set_test.cpp +++ b/common/set_test.cpp @@ -7,7 +7,12 @@ #include #include +#include #include +#include +#include +#include +#include #include #include @@ -19,6 +24,7 @@ namespace { using RawHashtable::IndexKeyContext; using RawHashtable::MoveOnlyTestData; using RawHashtable::TestData; +using ::testing::UnorderedElementsAre; using ::testing::UnorderedElementsAreArray; template @@ -26,7 +32,9 @@ auto ExpectSetElementsAre(SetT&& s, MatcherRangeT element_matchers) -> void { // Collect the elements into a container. using KeyT = std::remove_reference::type::KeyT; std::vector> entries; - s.ForEach([&entries](KeyT& k) { entries.push_back(std::ref(k)); }); + for (auto& k : s.entries()) { + entries.push_back(std::ref(k)); + } // Use the GoogleMock unordered container matcher to validate and show errors // on wrong elements. @@ -176,6 +184,8 @@ TYPED_TEST(SetTest, Move) { SetT other_s1 = std::move(s); ExpectSetElementsAre(other_s1, MakeElements(llvm::seq(1, 24))); + // A moved-from set has a size but no storage, and must iterate as empty. + EXPECT_EQ(s.entries().begin(), s.entries().end()); // Add some more elements. for (int i : llvm::seq(24, 32)) { @@ -432,5 +442,175 @@ TEST(SetContextTest, Basic) { ExpectSetElementsAre(s, MakeElements(llvm::seq(1, 512))); } +TYPED_TEST(SetTest, Range) { + using SetT = TypeParam; + using Range = decltype(std::declval().entries()); + using Iter = typename Range::Iterator; + + static_assert(std::forward_iterator); + static_assert(std::same_as().begin()), Iter>); + static_assert(std::same_as().end()), Iter>); + static_assert(std::ranges::forward_range); + static_assert(std::ranges::common_range); + + SetT s; + EXPECT_EQ(s.entries().begin(), s.entries().end()); + for (const auto& k : s.entries()) { + static_cast(k); + FAIL() << "Empty set range should have no elements"; + } + + for (int i = 1; i <= 5; ++i) { + s.Insert(i); + } + + // Range-for traversal by const ref. + int count = 0; + for (const auto& k : s.entries()) { + EXPECT_GE(k, 1); + EXPECT_LE(k, 5); + ++count; + } + EXPECT_EQ(count, 5); + + // Direct GMock container matching. + EXPECT_THAT(s.entries(), UnorderedElementsAre(1, 2, 3, 4, 5)); + + // Const view range iteration. + using KeyT = typename SetT::KeyT; + using KeyContextT = typename SetT::KeyContextT; + SetView cv = s; + int cv_count = 0; + for (const auto& k : cv.entries()) { + static_assert(std::is_const_v>); + EXPECT_GE(k, 1); + EXPECT_LE(k, 5); + ++cv_count; + } + EXPECT_EQ(cv_count, 5); + EXPECT_THAT(cv.entries(), UnorderedElementsAre(1, 2, 3, 4, 5)); + + // Explicit iterator traversal, dereference, and post-increment. + auto r = s.entries(); + int iter_count = 0; + for (auto it = r.begin(); it != r.end(); ++it) { + EXPECT_NE(*it, 0); + ++iter_count; + } + EXPECT_EQ(iter_count, 5); + + auto it = r.begin(); + auto prev = it++; + EXPECT_NE(it, prev); +} + +TYPED_TEST(MoveOnlySetTest, Range) { + TypeParam s; + s.Insert(1); + s.Insert(2); + + int count = 0; + for (const auto& k : s.entries()) { + EXPECT_GT(k.value, 0); + ++count; + } + EXPECT_EQ(count, 2); +} + +#ifndef NDEBUG +TEST(SetDeathTest, MutateDuringIterationFails) { + EXPECT_DEATH(([] { + Set s; + s.Insert(1); + auto range = s.entries(); + s.Insert(2); + }()), + "Hashtable mutated during iteration"); +} +#endif + +// A range outlives the *view* it was built from: views don't own storage, and +// the range copies the view rather than pointing at it. +TEST(SetTest, RangeOutlivesTemporaryView) { + Set s; + s.Insert(1); + + auto make_view = [&s]() -> SetView { return s; }; + auto range = make_view().entries(); + EXPECT_THAT(range, UnorderedElementsAre(1)); +} + +#ifdef NDEBUG +// Release iteration state is two end pointers, a group offset, and the +// present-bit mask; it needs to stay small enough to live in registers across +// the loop. Debug builds add the randomized walk and mutation-check state. +static_assert(sizeof(Set::Range::Iterator) <= 4 * sizeof(void*)); +#endif + +// Forward ranges guarantee multi-pass: `begin()` must be a pure function of the +// range. Debug builds draw their traversal entropy when the range is +// constructed rather than in `begin()` precisely so that repeated calls start +// from the same group. +TEST(SetTest, RangeIsMultiPass) { + Set s; + for (int i = 1; i <= 64; ++i) { + s.Insert(i); + } + + auto range = s.entries(); + EXPECT_EQ(range.begin(), range.begin()); + + // Two passes over the same range must agree on both the keys visited and the + // order they're visited in. + std::vector first; + for (int k : range) { + first.push_back(k); + } + std::vector second; + for (int k : range) { + second.push_back(k); + } + EXPECT_EQ(first, second); + EXPECT_EQ(static_cast(first.size()), 64); +} + +// Whatever order a range picks, it has to be a genuine permutation of the +// table. Debug builds additionally vary that order between ranges over the same +// table so that callers can't come to depend on it. +TEST(SetTest, TraversalOrderIsAVaryingPermutation) { + Set s; + std::vector inserted; + // Enough keys to populate every group of the small storage. + for (int i = 0; i < 36; ++i) { + int key = i * 17 + 7; + EXPECT_TRUE(s.Insert(key).is_inserted()); + inserted.push_back(key); + } + + std::set> distinct_orders; + for (int i = 0; i < 64; ++i) { + std::vector visited; + for (int k : s.entries()) { + visited.push_back(k); + } + // A walk that skipped a group would drop keys and one that revisited a + // group would duplicate them, so comparing as a multiset covers both. This + // is what makes an odd group stride a valid traversal. + EXPECT_THAT(visited, UnorderedElementsAreArray(inserted)); + distinct_orders.insert(visited); + } + +#ifndef NDEBUG + // Debug builds randomize both the starting group and the stride, so across + // this many ranges we should see more than the two orders (pure forward and + // pure reverse) that a simple direction flip would produce. + EXPECT_GT(distinct_orders.size(), 2) + << "Debug traversal order does not appear to be randomized."; +#else + // Release builds always scan the groups in order. + EXPECT_EQ(distinct_orders.size(), 1); +#endif +} + } // namespace } // namespace Carbon diff --git a/toolchain/benchmarking/source_gen.cpp b/toolchain/benchmarking/source_gen.cpp index cc3d943dd096..c1e2c196caa7 100644 --- a/toolchain/benchmarking/source_gen.cpp +++ b/toolchain/benchmarking/source_gen.cpp @@ -554,16 +554,13 @@ auto SourceGen::AppendUniqueIdentifiers( // Append all the identifiers directly out of the set. We make no guarantees // about the relative order so we just use the non-deterministic order of the // set and avoid additional storage. - // - // TODO: It's awkward the `ForEach` here can't early-exit. This just walks the - // whole set which is harmless if inefficient. We should add early exiting - // the loop support to `Set` and update this code. - unique_idents.ForEach([&](llvm::StringRef ident) { - if (number > 0) { - dest.push_back(ident); - --number; + for (llvm::StringRef ident : unique_idents.entries()) { + if (number == 0) { + break; } - }); + dest.push_back(ident); + --number; + } CARBON_CHECK(number == 0); } diff --git a/toolchain/check/scope_stack.cpp b/toolchain/check/scope_stack.cpp index fc40b718af8c..a7d392bcb42d 100644 --- a/toolchain/check/scope_stack.cpp +++ b/toolchain/check/scope_stack.cpp @@ -143,7 +143,7 @@ auto ScopeStack::Pop(bool check_unused) -> void { // TODO: Multiple diagnostics on same line has non-deterministic order. // Add second sort key in diagnostics sorting. - scope.names.ForEach([&, check_unused](SemIR::NameId name_id) { + for (SemIR::NameId name_id : scope.names.entries()) { auto& lexical_results = lexical_lookup_.Get(name_id); CARBON_CHECK(lexical_results.back().scope_index == scope.index, "Inconsistent scope index for name {0}", name_id); @@ -151,7 +151,7 @@ auto ScopeStack::Pop(bool check_unused) -> void { CheckUnusedBinding(*context_, name_id, lexical_results.back()); } lexical_results.pop_back(); - }); + } if (!scope.is_lexical_scope()) { CARBON_CHECK(non_lexical_scope_stack_.back().scope_index == scope.index); @@ -375,7 +375,7 @@ auto ScopeStack::Suspend() -> SuspendedScope { result.suspended_items.reserve(result.entry.num_names + peek_compile_time_bindings.size()); - result.entry.names.ForEach([&](SemIR::NameId name_id) { + for (SemIR::NameId name_id : result.entry.names.entries()) { auto suspended = lexical_lookup_.Suspend(name_id); CARBON_CHECK(suspended.index != SuspendedScope::ScopeItem::IndexForCompileTimeBinding); @@ -384,7 +384,7 @@ auto ScopeStack::Suspend() -> SuspendedScope { .inst_id = suspended.inst_id, .is_decl_reachable = suspended.is_decl_reachable, .use_loc_id = suspended.use_loc_id}); - }); + } CARBON_CHECK(static_cast(result.suspended_items.size()) == result.entry.num_names); diff --git a/toolchain/lower/specific_coalescer.cpp b/toolchain/lower/specific_coalescer.cpp index 40582bc232a4..2094883415ba 100644 --- a/toolchain/lower/specific_coalescer.cpp +++ b/toolchain/lower/specific_coalescer.cpp @@ -70,13 +70,12 @@ auto SpecificCoalescer::CoalesceEquivalentSpecifics( // When processing equivalences, we may change the canonical specific // multiple times, so we don't delete replaced specifics until the // end. - visited_equivalent_specifics.ForEach( - [&](std::pair - equivalent_entry) { - CARBON_VLOG("Found equivalent specifics: {0}, {1}", - equivalent_entry.first, equivalent_entry.second); - ProcessSpecificEquivalence(equivalent_entry); - }); + for (const auto& equivalent_entry : + visited_equivalent_specifics.entries()) { + CARBON_VLOG("Found equivalent specifics: {0}, {1}", + equivalent_entry.first, equivalent_entry.second); + ProcessSpecificEquivalence(equivalent_entry); + } // Removed the replaced specific from the list of emitted specifics. // Only the top level, since the others are somewhere else in the diff --git a/toolchain/testing/coverage_helper.h b/toolchain/testing/coverage_helper.h index 668f7c51eca9..28eef1320871 100644 --- a/toolchain/testing/coverage_helper.h +++ b/toolchain/testing/coverage_helper.h @@ -72,8 +72,9 @@ auto TestKindCoverage(const std::string& manifest_path, << llvm::join(missing_kinds, Bullet); llvm::SmallVector unexpected_matches; - covered_kinds.ForEach( - [&](const std::string& match) { unexpected_matches.push_back(match); }); + for (const std::string& match : covered_kinds.entries()) { + unexpected_matches.push_back(match); + } llvm::sort(unexpected_matches); EXPECT_TRUE(unexpected_matches.empty()) << "Matched things that aren't in the kind list:" << Bullet