mirror of
https://github.com/carbon-language/carbon-lang.git
synced 2026-10-05 14:51:03 +01:00
Remove another hashtable iteraiton order dependency. (#4070)
Name scopes store the names in their scope in a `DenseMap`. Several places reasonably avoid depending on the iteration order by sorting the names -- they're in the formatting code path where that's a solid approach. Unfortunately, when we're importing one scope into another, we also need to walk the entire scope and do something for each name. =[ This doesn't seem like a great place to sort things to stabilize them. I've switched to a fairly simplistic solution of having a vector of name entries that can be iterated stably, and a separate map for lookups. I didn't use the set-of-indices trick here because it's not clear that's the right trade-off for a scope: likely a lot of small scopes here with relatively hot name lookups. And the key here isn't a large or dynamically sized thing that we're canonicalizing, it's a `NameId`. That made me lean towards duplicating the name in the hashtable for lookup and the vector for iteration. I thought about a fancy approach of sorting the hashtable keys by their values (the indices), but that would still require a bit of copying and more code. I also thought a bit about other optimizations, but decided to leave a comment for now -- it's not obvious to me exactly how hot this is and whether it's better served by faster lookups, being more memory dense, etc. And that might involve more of an SOA layout change or some other approach. Rather than do that here, and especially before switching hashtables, I stuck with a simple approach to address the ordering. --------- Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This commit is contained in:
co-authored by
Richard Smith
parent
af6312a3aa
commit
b70cfd0be9
@@ -147,9 +147,12 @@ auto DeclNameStack::AddName(NameContext name_context, SemIR::InstId target_id,
|
||||
context_->AddExport(target_id);
|
||||
}
|
||||
|
||||
auto [_, success] = name_scope.names.insert(
|
||||
{name_context.unresolved_name_id,
|
||||
{.inst_id = target_id, .access_kind = access_kind}});
|
||||
int scope_index = name_scope.names.size();
|
||||
name_scope.names.push_back({.name_id = name_context.unresolved_name_id,
|
||||
.inst_id = target_id,
|
||||
.access_kind = access_kind});
|
||||
auto [_, success] = name_scope.name_map.insert(
|
||||
{name_context.unresolved_name_id, scope_index});
|
||||
CARBON_CHECK(success)
|
||||
<< "Duplicate names should have been resolved previously: "
|
||||
<< name_context.unresolved_name_id << " in "
|
||||
|
||||
Reference in New Issue
Block a user