Skip to content

Commit 64139e5

Browse files
authored
Stop using Map for the cache in InstFingerprinter (#6019)
This takes the debug runtime of `toolchain/check/testdata/interop/cpp/function/arithmetic_types_bridged.carbon` from 4.7s down to about 4s (so 15% faster overall). There's still lots of room to improve this test which seems to be hitting lots of pathological behaviour, but InstNamer is 30% of the runtime, with fingerprinting's `InstFingerprinter::GetOrCompute` consuming 10% of cycles. We reduce its impact by using a vector of vectors instead of a Map for the cache of fingerprints. After this change InstNamer drops below 24% of the runtime. Also move the instruction name when giving it to `AllocateName` since it receives std::string by value, though this doesn't show up in the profile for the test.
1 parent 471b394 commit 64139e5

21 files changed

+118
-44
lines changed

toolchain/base/fixed_size_value_store.h

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55
#ifndef CARBON_TOOLCHAIN_BASE_FIXED_SIZE_VALUE_STORE_H_
66
#define CARBON_TOOLCHAIN_BASE_FIXED_SIZE_VALUE_STORE_H_
77

8+
#include <concepts>
9+
#include <type_traits>
10+
811
#include "common/check.h"
912
#include "llvm/ADT/SmallVector.h"
1013
#include "llvm/ADT/StringRef.h"
@@ -51,6 +54,19 @@ class FixedSizeValueStore {
5154
return store;
5255
}
5356

57+
// Makes a ValueStore of the specified size, initialized to values returned
58+
// from a callback. This allows initializing non-copyable values.
59+
static auto MakeWithExplicitSizeFrom(
60+
size_t size, llvm::function_ref<auto()->ValueT> callable)
61+
-> FixedSizeValueStore {
62+
FixedSizeValueStore store;
63+
store.values_.reserve(size);
64+
for (auto _ : llvm::seq(size)) {
65+
store.values_.push_back(callable());
66+
}
67+
return store;
68+
}
69+
5470
// Makes a ValueStore of the same size as a source `ValueStoreT`. This is
5571
// the safest constructor to use, since it ensures everything's initialized to
5672
// a default, and verifies a matching `IdT` for the size.

toolchain/check/check.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,7 @@ static auto BuildApiMapAndDiagnosePackaging(
332332

333333
// Handles printing of formatted SemIR.
334334
static auto MaybeDumpFormattedSemIR(
335-
const SemIR::File& sem_ir,
335+
const SemIR::File& sem_ir, int total_ir_count,
336336
Parse::GetTreeAndSubtreesFn tree_and_subtrees_getter, bool include_in_dumps,
337337
const CheckParseTreesOptions& options) -> void {
338338
bool dump = options.dump_stream && include_in_dumps;
@@ -351,7 +351,7 @@ static auto MaybeDumpFormattedSemIR(
351351
options.dump_sem_ir_ranges !=
352352
CheckParseTreesOptions::DumpSemIRRanges::Ignore &&
353353
tokens.has_dump_sem_ir_ranges();
354-
SemIR::Formatter formatter(&sem_ir, tree_and_subtrees_getter,
354+
SemIR::Formatter formatter(&sem_ir, total_ir_count, tree_and_subtrees_getter,
355355
options.include_in_dumps, use_dump_sem_ir_ranges);
356356
formatter.Format();
357357
if (options.vlog_stream) {
@@ -387,7 +387,8 @@ static auto MaybeDumpSemIR(
387387
}
388388

389389
MaybeDumpFormattedSemIR(
390-
*unit.sem_ir, tree_and_subtrees_getters.Get(unit.sem_ir->check_ir_id()),
390+
*unit.sem_ir, units.size(),
391+
tree_and_subtrees_getters.Get(unit.sem_ir->check_ir_id()),
391392
include_in_dumps, options);
392393
}
393394
}

toolchain/check/check.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ struct Unit {
2727

2828
// The unit's SemIR, provided as empty and filled in by CheckParseTrees.
2929
SemIR::File* sem_ir;
30+
// The total number of files.
31+
int total_ir_count;
3032

3133
// Storage for the unit's Clang AST. The unique_ptr should start empty, and
3234
// can be assigned as part of checking.

toolchain/check/check_unit.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -63,15 +63,15 @@ CheckUnit::CheckUnit(
6363
: unit_and_imports_(unit_and_imports),
6464
tree_and_subtrees_getter_(tree_and_subtrees_getters->Get(
6565
unit_and_imports->unit->sem_ir->check_ir_id())),
66-
total_ir_count_(tree_and_subtrees_getters->size()),
6766
fs_(std::move(fs)),
6867
clang_invocation_(std::move(clang_invocation)),
6968
emitter_(&unit_and_imports_->err_tracker, tree_and_subtrees_getters,
7069
unit_and_imports_->unit->sem_ir),
7170
context_(&emitter_, tree_and_subtrees_getter_,
7271
unit_and_imports_->unit->sem_ir,
73-
GetImportedIRCount(unit_and_imports), total_ir_count_,
74-
gen_implicit_type_impls, vlog_stream) {}
72+
GetImportedIRCount(unit_and_imports),
73+
unit_and_imports_->unit->total_ir_count, gen_implicit_type_impls,
74+
vlog_stream) {}
7575

7676
auto CheckUnit::Run() -> void {
7777
Timings::ScopedTiming timing(unit_and_imports_->unit->timings, "check");
@@ -195,7 +195,7 @@ auto CheckUnit::CollectTransitiveImports(SemIR::InstId import_decl_id,
195195
// exported to this IR.
196196
auto ir_to_result_index =
197197
FixedSizeValueStore<SemIR::CheckIRId, int>::MakeWithExplicitSize(
198-
total_ir_count_, -1);
198+
unit_and_imports_->unit->total_ir_count, -1);
199199

200200
// First add direct imports. This means that if an entity is imported both
201201
// directly and indirectly, the import path will reflect the direct import.

toolchain/check/check_unit.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,8 +187,6 @@ class CheckUnit {
187187

188188
UnitAndImports* unit_and_imports_;
189189
Parse::GetTreeAndSubtreesFn tree_and_subtrees_getter_;
190-
// The number of IRs being checked in total.
191-
int total_ir_count_;
192190
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs_;
193191
std::shared_ptr<clang::CompilerInvocation> clang_invocation_;
194192

toolchain/check/context.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ Context::Context(DiagnosticEmitterBase* emitter,
2020
: emitter_(emitter),
2121
tree_and_subtrees_getter_(tree_and_subtrees_getter),
2222
sem_ir_(sem_ir),
23+
total_ir_count_(total_ir_count),
2324
gen_implicit_type_impls_(gen_implicit_type_impls),
2425
vlog_stream_(vlog_stream),
2526
node_stack_(sem_ir->parse_tree(), vlog_stream),
@@ -33,7 +34,7 @@ Context::Context(DiagnosticEmitterBase* emitter,
3334
vtable_stack_("vtable_stack_", *sem_ir, vlog_stream),
3435
check_ir_map_(
3536
FixedSizeValueStore<SemIR::CheckIRId, SemIR::ImportIRId>::
36-
MakeWithExplicitSize(total_ir_count, SemIR::ImportIRId::None)),
37+
MakeWithExplicitSize(total_ir_count_, SemIR::ImportIRId::None)),
3738
global_init_(this),
3839
region_stack_([this](SemIR::LocId loc_id, std::string label) {
3940
TODO(loc_id, label);

toolchain/check/context.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,8 @@ class Context {
313313

314314
// The SemIR::File being added to.
315315
SemIR::File* sem_ir_;
316+
// The total number of files.
317+
int total_ir_count_;
316318

317319
// Whether to generate standard `impl`s for types, such as `Core.Destroy`; see
318320
// `CheckParseTreesOptions`.

toolchain/driver/compile_subcommand.cpp

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -436,8 +436,8 @@ class MultiUnitCache;
436436
class CompilationUnit {
437437
public:
438438
// `driver_env`, `options`, and `consumer` must be non-null.
439-
explicit CompilationUnit(SemIR::CheckIRId check_ir_id, DriverEnv* driver_env,
440-
const CompileOptions* options,
439+
explicit CompilationUnit(SemIR::CheckIRId check_ir_id, int total_ir_count,
440+
DriverEnv* driver_env, const CompileOptions* options,
441441
Diagnostics::Consumer* consumer,
442442
llvm::StringRef input_filename);
443443

@@ -499,6 +499,8 @@ class CompilationUnit {
499499

500500
// The index of the unit amongst all units.
501501
SemIR::CheckIRId check_ir_id_;
502+
// The number of units in total.
503+
int total_ir_count_;
502504

503505
DriverEnv* driver_env_;
504506
const CompileOptions* options_;
@@ -619,11 +621,12 @@ class MultiUnitCache {
619621
} // namespace
620622

621623
CompilationUnit::CompilationUnit(SemIR::CheckIRId check_ir_id,
622-
DriverEnv* driver_env,
624+
int total_ir_count, DriverEnv* driver_env,
623625
const CompileOptions* options,
624626
Diagnostics::Consumer* consumer,
625627
llvm::StringRef input_filename)
626628
: check_ir_id_(check_ir_id),
629+
total_ir_count_(total_ir_count),
627630
driver_env_(driver_env),
628631
options_(options),
629632
input_filename_(input_filename),
@@ -724,6 +727,7 @@ auto CompilationUnit::GetCheckUnit() -> Check::Unit {
724727
.value_stores = &value_stores_,
725728
.timings = timings_ ? &*timings_ : nullptr,
726729
.sem_ir = &*sem_ir_,
730+
.total_ir_count = total_ir_count_,
727731
.clang_ast_unit = &clang_ast_unit_};
728732
}
729733

@@ -757,7 +761,7 @@ auto CompilationUnit::RunLower() -> void {
757761
}
758762
module_ = Lower::LowerToLLVM(*llvm_context_, driver_env_->fs,
759763
cache_->tree_and_subtrees_getters(), *sem_ir_,
760-
options);
764+
total_ir_count_, options);
761765
});
762766
}
763767

@@ -955,16 +959,18 @@ auto CompileSubcommand::Run(DriverEnv& driver_env) -> DriverResult {
955959

956960
// Prepare CompilationUnits before building scope exit handlers.
957961
llvm::SmallVector<std::unique_ptr<CompilationUnit>> units;
962+
int total_unit_count = prelude.size() + options_.input_filenames.size();
958963
int unit_index = -1;
959964
auto unit_builder = [&](llvm::StringRef filename) {
960965
++unit_index;
961-
return std::make_unique<CompilationUnit>(SemIR::CheckIRId(unit_index),
962-
&driver_env, &options_,
963-
&driver_env.consumer, filename);
966+
return std::make_unique<CompilationUnit>(
967+
SemIR::CheckIRId(unit_index), total_unit_count, &driver_env, &options_,
968+
&driver_env.consumer, filename);
964969
};
965970
llvm::append_range(units, llvm::map_range(prelude, unit_builder));
966971
llvm::append_range(units,
967972
llvm::map_range(options_.input_filenames, unit_builder));
973+
CARBON_CHECK(units.size() == static_cast<size_t>(total_unit_count));
968974

969975
// Add the cache to all units. This must be done after all units are created.
970976
MultiUnitCache cache(&options_, units);

toolchain/language_server/context.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,7 @@ auto Context::File::SetText(Context& context, std::optional<int64_t> version,
154154
.value_stores = value_stores_.get(),
155155
.timings = nullptr,
156156
.sem_ir = &sem_ir,
157+
.total_ir_count = 1,
157158
.clang_ast_unit = &clang_ast_unt}}};
158159

159160
auto getter = [this]() -> const Parse::TreeAndSubtrees& {
@@ -163,7 +164,7 @@ auto Context::File::SetText(Context& context, std::optional<int64_t> version,
163164
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs =
164165
llvm::vfs::getRealFileSystem();
165166

166-
// TODO: Include the prelude.
167+
// TODO: Include the prelude. Make sure `total_ir_count` includes the files.
167168
Check::CheckParseTreesOptions check_options;
168169
check_options.vlog_stream = context.vlog_stream();
169170
auto getters =

toolchain/lower/context.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@ Context::Context(
1818
llvm::LLVMContext* llvm_context,
1919
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs, bool want_debug_info,
2020
const Parse::GetTreeAndSubtreesStore* tree_and_subtrees_getters,
21-
llvm::StringRef module_name, llvm::raw_ostream* vlog_stream)
21+
llvm::StringRef module_name, int total_ir_count,
22+
llvm::raw_ostream* vlog_stream)
2223
: llvm_context_(llvm_context),
2324
llvm_module_(std::make_unique<llvm::Module>(module_name, *llvm_context)),
2425
file_system_(std::move(fs)),
@@ -28,7 +29,8 @@ Context::Context(
2829
? BuildDICompileUnit(module_name, *llvm_module_, di_builder_)
2930
: nullptr),
3031
tree_and_subtrees_getters_(tree_and_subtrees_getters),
31-
vlog_stream_(vlog_stream) {}
32+
vlog_stream_(vlog_stream),
33+
total_ir_count_(total_ir_count) {}
3234

3335
auto Context::GetFileContext(const SemIR::File* file,
3436
const SemIR::InstNamer* inst_namer)

0 commit comments

Comments
 (0)