Skip to content

Commit 3979af3

Browse files
danakjbricknerb
authored andcommitted
Make pointers in ValueStore stable across insertions (carbon-language#5576)
This avoids reallocating the backing buffer in ValueStore so that references into the ValueStore are never invalidated when adding new values. This works especially well since we never delete values from a ValueStore. The strategy used is to allocate chunks of a fixed size, and inserting into each chunk until it is full before allocating the next. The ValueStore starts with an initial allocated chunk in all cases, so that there is only a single indirection for adding and accessing values from this chunk. After it's full, additional chunks are allocated in a vector, so two indirections are required to add or access values in these chunks. This obviates the need for carbon-language#5529 as we no longer need to worry about holding pointers into a ValueStore. We introduce a Flatten operation for ranges. It flattens a "range over ranges over Ts" down to a "range over Ts". This allows us to make an range over the values in the ValueStore from a range over the chunks in the ValueStore. See https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html#method.flatten for inspiration for this name choice. Flatten is used in one other case where we were writing two levels of for loops to do the same thing. The `array_ref()` accessor is changed to `values()` and its now a range (typed as a `ValueStoreRange`) over all values as references (like ArrayRef was, but without random access). As pointers to a ValueStore can no longer be invalidated, we remove the ASAN poisoning feature and support from ValueStore. This may cause a regression in our compile benchmark of up to 5%, though that is close to or within the noise of the benchmark. We can look at ways to optimize things further in the future. Perhaps by tuning the chunk size further, or by making later chunks larger than earlier chunks, or other strategies.
1 parent 5493072 commit 3979af3

File tree

16 files changed

+305
-198
lines changed

16 files changed

+305
-198
lines changed

bazel/cc_toolchains/clang_cc_toolchain_config.bzl

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -643,22 +643,6 @@ def _impl(ctx):
643643
],
644644
)
645645

646-
# A feature that enables poisoning of value stores to detect use after
647-
# potential reallocation bugs.
648-
#
649-
# TODO: Remove this and leave poisoning always on once these bugs are
650-
# fixed.
651-
poison_value_stores = feature(
652-
name = "poison_value_stores",
653-
requires = [feature_set(["asan"])],
654-
flag_sets = [flag_set(
655-
actions = all_compile_actions,
656-
flag_groups = [flag_group(flags = [
657-
"-DCARBON_POISON_VALUE_STORES=1",
658-
])],
659-
)],
660-
)
661-
662646
fuzzer = feature(
663647
name = "fuzzer",
664648
flag_sets = [flag_set(
@@ -1153,7 +1137,6 @@ def _impl(ctx):
11531137
asan,
11541138
asan_min_size,
11551139
enable_in_fastbuild,
1156-
poison_value_stores,
11571140
fuzzer,
11581141
layering_check,
11591142
module_maps,

toolchain/base/BUILD

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,10 @@ cc_library(
8686

8787
cc_library(
8888
name = "value_store",
89-
hdrs = ["value_store.h"],
89+
hdrs = [
90+
"value_store.h",
91+
"value_store_chunk.h",
92+
],
9093
deps = [
9194
":mem_usage",
9295
":yaml",

toolchain/base/int.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -346,8 +346,8 @@ class IntStore {
346346
// into the ID space.
347347
auto OutputYaml() const -> Yaml::OutputMapping;
348348

349-
auto array_ref() const -> llvm::ArrayRef<llvm::APInt> {
350-
return values_.array_ref();
349+
auto values() const [[clang::lifetimebound]] -> auto {
350+
return values_.values();
351351
}
352352
auto size() const -> size_t { return values_.size(); }
353353

0 commit comments

Comments
 (0)