Avoid relying on a hashtable iteration order. (#3960)

The toolchain was iterating a map of package name to import list in a
couple of places to do things beyond counting or other order-invariant
operations. The result was that the specific hashtable iteration order
influenced the order of SemIR generated (and other behaviors like
diagnostic emission I suspect, but the source location sorting probably
hid this). Switching to a hashtable implementation with any order
seeding that changes run-to-run immediately shows the SemIR order
fluctuating without this.

This PR fixes that by instead accumulating the package imports data in a
vector and using a map to vector indices. This is also slightly more
efficient, although that seems unlikely to be an important factor here.

There was only one insertion point so I've just hand coded the
management of the indices and map, but happy to take a different
approach or use an abstraction here if desired.

One awkward aspect of this is that some of the loops need access to the
package name as well. I've just added storage for that as these seem
unlikely to be huge arrays of 10s of 1000s of imported packages, so the
double storage of the identifier ID seems likely OK. But again, happy to
take a different approach here if desired. It also seems like it might
be possible to work out the identifier from the node, but I kept the
patch more direct for simplicity.

I've also not used the LLVM `MapVector` abstraction of this pattern.
This was mostly to avoid adding another layer of abstractions to our
data structures, and because this is the first time we've hit this
really. My experience is also that it is reasonably often that there is
a more efficient way to orient the vector and map than what is
automatically provided. But that may just be my experience.

toolchain/check/check.cpp

@@ -158,9 +158,12 @@ struct UnitInfo {
   struct PackageImports {
     // Use the constructor so that the SmallVector is only constructed
     // as-needed.
-    explicit PackageImports(Parse::ImportDirectiveId node_id)
-        : node_id(node_id) {}
+    explicit PackageImports(IdentifierId package_id,
+                            Parse::ImportDirectiveId node_id)
+        : package_id(package_id), node_id(node_id) {}
 
+    // The identifier of the imported package.
+    IdentifierId package_id;
     // The first `import` directive in the file, which declared the package's
     // identifier (even if the import failed). Used for associating diagnostics
     // not specific to a single import.
@@ -187,11 +190,14 @@ struct UnitInfo {
   ErrorTrackingDiagnosticConsumer err_tracker;
   DiagnosticEmitter<Parse::NodeLoc> emitter;
 
-  // A map of package names to outgoing imports. If a package includes
-  // unavailable library imports, it has an entry with has_load_error set.
-  // Invalid imports (for example, `import Main;`) aren't added because they
-  // won't add identifiers to name lookup.
-  llvm::DenseMap<IdentifierId, PackageImports> package_imports_map;
+  // List of the outgoing imports. If a package includes unavailable library
+  // imports, it has an entry with has_load_error set. Invalid imports (for
+  // example, `import Main;`) aren't added because they won't add identifiers to
+  // name lookup.
+  llvm::SmallVector<PackageImports> package_imports;
+
+  // A map of the package names to the outgoing imports above.
+  llvm::DenseMap<IdentifierId, int32_t> package_imports_map;
 
   // The remaining number of imports which must be checked before this unit can
   // be processed.
@@ -212,22 +218,25 @@ static auto ImportCurrentPackage(Context& context, UnitInfo& unit_info,
                                  SemIR::InstId package_inst_id,
                                  SemIR::TypeId namespace_type_id) -> void {
   // Add imports from the current package.
-  auto self_import = unit_info.package_imports_map.find(IdentifierId::Invalid);
-  if (self_import == unit_info.package_imports_map.end()) {
+  auto self_import_map_it =
+      unit_info.package_imports_map.find(IdentifierId::Invalid);
+  if (self_import_map_it == unit_info.package_imports_map.end()) {
     // Push the scope; there are no names to add.
     context.scope_stack().Push(package_inst_id, SemIR::NameScopeId::Package);
     return;
   }
+  UnitInfo::PackageImports& self_import =
+      unit_info.package_imports[self_import_map_it->second];
 
   // Track whether an IR was imported in full, including `export import`. This
   // distinguishes from IRs that are indirectly added without all names being
   // exported to this IR.
   llvm::SmallVector<bool> imported_irs(total_ir_count, false);
-  if (self_import->second.has_load_error) {
+  if (self_import.has_load_error) {
     context.name_scopes().Get(SemIR::NameScopeId::Package).has_error = true;
   }
 
-  for (const auto& import : self_import->second.imports) {
+  for (const auto& import : self_import.imports) {
     const auto& import_sem_ir = **import.unit_info->unit->sem_ir;
 
     auto& imported_ir = imported_irs[import_sem_ir.check_ir_id().index];
@@ -288,7 +297,7 @@ static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info,
   // First create the constant values map for all imported IRs. We'll populate
   // these with mappings for namespaces as we go.
   size_t num_irs = 0;
-  for (auto& [_, package_imports] : unit_info.package_imports_map) {
+  for (auto& package_imports : unit_info.package_imports) {
     num_irs += package_imports.imports.size();
   }
   if (!unit_info.api_for_impl) {
@@ -334,8 +343,8 @@ static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info,
                        namespace_type_id);
   CARBON_CHECK(context.scope_stack().PeekIndex() == ScopeIndex::Package);
 
-  for (auto& [package_id, package_imports] : unit_info.package_imports_map) {
-    if (!package_id.is_valid()) {
+  for (auto& package_imports : unit_info.package_imports) {
+    if (!package_imports.package_id.is_valid()) {
       // Current package is handled above.
       continue;
     }
@@ -348,9 +357,9 @@ static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info,
       CARBON_CHECK(!import.names.is_export)
           << "Imports from other packages can't be exported.";
     }
-    ImportLibrariesFromOtherPackage(context, namespace_type_id,
-                                    package_imports.node_id, package_id,
-                                    import_irs, package_imports.has_load_error);
+    ImportLibrariesFromOtherPackage(
+        context, namespace_type_id, package_imports.node_id,
+        package_imports.package_id, import_irs, package_imports.has_load_error);
   }
 }
 
@@ -998,14 +1007,20 @@ static auto TrackImport(
     return;
   }
 
-  // Get the package imports.
-  auto package_imports_it = unit_info.package_imports_map
-                                .try_emplace(import.package_id, import.node_id)
-                                .first;
+  // Get the package imports, or create them if this is the first.
+  auto [package_imports_map_it, is_inserted] =
+      unit_info.package_imports_map.insert(
+          {import.package_id, unit_info.package_imports.size()});
+  if (is_inserted) {
+    unit_info.package_imports.push_back(
+        UnitInfo::PackageImports(import.package_id, import.node_id));
+  }
+  UnitInfo::PackageImports& package_imports =
+      unit_info.package_imports[package_imports_map_it->second];
 
   if (auto api = api_map.find(import_key); api != api_map.end()) {
     // Add references between the file and imported api.
-    package_imports_it->second.imports.push_back({import, api->second});
+    package_imports.imports.push_back({import, api->second});
     ++unit_info.imports_remaining;
     api->second->incoming_imports.push_back(&unit_info);
 
@@ -1016,7 +1031,7 @@ static auto TrackImport(
     }
   } else {
     // The imported api is missing.
-    package_imports_it->second.has_load_error = true;
+    package_imports.has_load_error = true;
     CARBON_DIAGNOSTIC(LibraryApiNotFound, Error,
                       "Corresponding API for '{0}' not found.", std::string);
     CARBON_DIAGNOSTIC(ImportNotFound, Error, "Imported API '{0}' not found.",
@@ -1196,8 +1211,7 @@ auto CheckParseTrees(llvm::MutableArrayRef<Unit> units, bool prelude_import,
     // TODO: Better identify cycles, maybe try to untangle them.
     for (auto& unit_info : unit_infos) {
       if (unit_info.imports_remaining > 0) {
-        for (auto& [package_id, package_imports] :
-             unit_info.package_imports_map) {
+        for (auto& package_imports : unit_info.package_imports) {
           for (auto* import_it = package_imports.imports.begin();
                import_it != package_imports.imports.end();) {
             if (*import_it->unit_info->unit->sem_ir) {

toolchain/check/testdata/class/cross_package_import.carbon

@@ -289,16 +289,16 @@ var c: Other.C = {};
 // CHECK:STDOUT:
 // CHECK:STDOUT: file {
 // CHECK:STDOUT:   package: <namespace> = namespace [template] {
-// CHECK:STDOUT:     .Other = %Other
 // CHECK:STDOUT:     .Core = %Core
+// CHECK:STDOUT:     .Other = %Other
 // CHECK:STDOUT:     .c = %c
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %Other: <namespace> = namespace [template] {}
 // CHECK:STDOUT:   %Core: <namespace> = namespace [template] {}
+// CHECK:STDOUT:   %Other: <namespace> = namespace [template] {}
 // CHECK:STDOUT:   %Other.ref: <namespace> = name_ref Other, %Other [template = %Other]
-// CHECK:STDOUT:   %import_ref.1: type = import_ref ir1, inst+2, loaded [template = constants.%C]
-// CHECK:STDOUT:   %import_ref.2 = import_ref ir1, inst+3, unloaded
-// CHECK:STDOUT:   %import_ref.3 = import_ref ir2, inst+2, unloaded
+// CHECK:STDOUT:   %import_ref.1: type = import_ref ir2, inst+2, loaded [template = constants.%C]
+// CHECK:STDOUT:   %import_ref.2 = import_ref ir2, inst+3, unloaded
+// CHECK:STDOUT:   %import_ref.3 = import_ref ir3, inst+2, unloaded
 // CHECK:STDOUT:   %C.ref: type = name_ref C, %import_ref.1 [template = constants.%C]
 // CHECK:STDOUT:   %c.var: ref C = var c
 // CHECK:STDOUT:   %c: ref C = bind_name c, %c.var

toolchain/check/testdata/impl/lookup/import.carbon

@@ -112,26 +112,26 @@ fn G(c: Impl.C) {
 // CHECK:STDOUT:
 // CHECK:STDOUT: file {
 // CHECK:STDOUT:   package: <namespace> = namespace [template] {
-// CHECK:STDOUT:     .Impl = %Impl
 // CHECK:STDOUT:     .Core = %Core
+// CHECK:STDOUT:     .Impl = %Impl
 // CHECK:STDOUT:     .G = %G.decl
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %Impl: <namespace> = namespace [template] {}
 // CHECK:STDOUT:   %Core: <namespace> = namespace [template] {}
-// CHECK:STDOUT:   %import_ref.1 = import_ref ir1, inst+14, unloaded
-// CHECK:STDOUT:   %import_ref.2: <associated F in HasF> = import_ref ir1, inst+11, loaded [template = constants.%.6]
-// CHECK:STDOUT:   %import_ref.3 = import_ref ir1, inst+4, unloaded
-// CHECK:STDOUT:   %import_ref.4 = import_ref ir1, inst+6, unloaded
-// CHECK:STDOUT:   %import_ref.5: <witness> = import_ref ir1, inst+22, loaded [template = constants.%.7]
-// CHECK:STDOUT:   %import_ref.6: type = import_ref ir1, inst+13, loaded [template = constants.%C]
+// CHECK:STDOUT:   %Impl: <namespace> = namespace [template] {}
+// CHECK:STDOUT:   %import_ref.1 = import_ref ir2, inst+14, unloaded
+// CHECK:STDOUT:   %import_ref.2: <associated F in HasF> = import_ref ir2, inst+11, loaded [template = constants.%.6]
+// CHECK:STDOUT:   %import_ref.3 = import_ref ir2, inst+4, unloaded
+// CHECK:STDOUT:   %import_ref.4 = import_ref ir2, inst+6, unloaded
+// CHECK:STDOUT:   %import_ref.5: <witness> = import_ref ir2, inst+22, loaded [template = constants.%.7]
+// CHECK:STDOUT:   %import_ref.6: type = import_ref ir2, inst+13, loaded [template = constants.%C]
 // CHECK:STDOUT:   %G.decl: G = fn_decl @G [template = constants.%struct.1] {
 // CHECK:STDOUT:     %Impl.ref: <namespace> = name_ref Impl, %Impl [template = %Impl]
 // CHECK:STDOUT:     %C.ref: type = name_ref C, %import_ref.6 [template = constants.%C]
 // CHECK:STDOUT:     %c.loc4_6.1: C = param c
 // CHECK:STDOUT:     @G.%c: C = bind_name c, %c.loc4_6.1
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %import_ref.7: type = import_ref ir1, inst+2, loaded [template = constants.%.2]
-// CHECK:STDOUT:   %import_ref.8 = import_ref ir1, inst+6, unloaded
+// CHECK:STDOUT:   %import_ref.7: type = import_ref ir2, inst+2, loaded [template = constants.%.2]
+// CHECK:STDOUT:   %import_ref.8 = import_ref ir2, inst+6, unloaded
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: interface @HasF {

toolchain/check/testdata/packages/cross_package_import.carbon

@@ -408,7 +408,7 @@ fn Other.G() {}
 // CHECK:STDOUT:   %import_ref.1: <namespace> = import_ref ir1, inst+2, loaded
 // CHECK:STDOUT:   %Other: <namespace> = namespace %import_ref.1, [template] {}
 // CHECK:STDOUT:   %Core: <namespace> = namespace [template] {}
-// CHECK:STDOUT:   %import_ref.2: F = import_ref ir2, inst+2, loaded [template = constants.%struct]
+// CHECK:STDOUT:   %import_ref.2: F = import_ref ir3, inst+2, loaded [template = constants.%struct]
 // CHECK:STDOUT:   %F.decl: F = fn_decl @F [template = constants.%struct] {}
 // CHECK:STDOUT: }
 // CHECK:STDOUT:

toolchain/check/testdata/packages/explicit_imports.carbon

@@ -65,11 +65,11 @@ import library "lib";
 // CHECK:STDOUT:
 // CHECK:STDOUT: file {
 // CHECK:STDOUT:   package: <namespace> = namespace [template] {
-// CHECK:STDOUT:     .Api = %Api
 // CHECK:STDOUT:     .Core = %Core
+// CHECK:STDOUT:     .Api = %Api
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %Api: <namespace> = namespace [template] {}
 // CHECK:STDOUT:   %Core: <namespace> = namespace [template] {}
+// CHECK:STDOUT:   %Api: <namespace> = namespace [template] {}
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: --- main_lib_api.carbon

toolchain/check/testdata/packages/fail_import_repeat.carbon

@@ -92,11 +92,11 @@ import library default;
 // CHECK:STDOUT:
 // CHECK:STDOUT: file {
 // CHECK:STDOUT:   package: <namespace> = namespace [template] {
-// CHECK:STDOUT:     .Api = %Api
 // CHECK:STDOUT:     .Core = %Core
+// CHECK:STDOUT:     .Api = %Api
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %Api: <namespace> = namespace [template] {}
 // CHECK:STDOUT:   %Core: <namespace> = namespace [template] {}
+// CHECK:STDOUT:   %Api: <namespace> = namespace [template] {}
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: --- fail_default_import.carbon