Commit 91b897ef58

Andrew Kelley <andrew@ziglang.org>
2023-11-12 23:45:10
rework memory management of Module.Namespace hash maps
The motivating problem here was a memory leak in the hash maps of Module.Namespace. The commit deletes more of the legacy incremental compilation implementation. It had things like use of orderedRemove and trying to do too much OOP-style creation and deletion of objects. Instead, this commit iterates over all the namespaces on Module deinit and calls deinit on the hash map fields. This logic is much simpler to reason about. Similarly, change global inline assembly to an array hash map since iterating over the values is a primary use of it, and clean up the remaining values on Module deinit, solving another memory leak. After this there are no more memory leaks remaining when using the x86 backend in a libc-less compiler.
1 parent 53500a5
src/codegen/c.zig
@@ -2506,8 +2506,9 @@ pub fn genTypeDecl(
 }
 
 pub fn genGlobalAsm(mod: *Module, writer: anytype) !void {
-    var it = mod.global_assembly.valueIterator();
-    while (it.next()) |asm_source| try writer.print("__asm({s});\n", .{fmtStringLiteral(asm_source.*, null)});
+    for (mod.global_assembly.values()) |asm_source| {
+        try writer.print("__asm({s});\n", .{fmtStringLiteral(asm_source, null)});
+    }
 }
 
 pub fn genErrDecls(o: *Object) !void {
src/codegen/llvm.zig
@@ -1121,8 +1121,9 @@ pub const Object = struct {
         const mod = object.module;
 
         const writer = object.builder.setModuleAsm();
-        var it = mod.global_assembly.valueIterator();
-        while (it.next()) |assembly| try writer.print("{s}\n", .{assembly.*});
+        for (mod.global_assembly.values()) |assembly| {
+            try writer.print("{s}\n", .{assembly});
+        }
         try object.builder.finishModuleAsm();
     }
 
src/Compilation.zig
@@ -2522,18 +2522,6 @@ pub fn update(comp: *Compilation, main_progress_node: *std.Progress.Node) !void
             try module.populateTestFunctions(main_progress_node);
         }
 
-        // Process the deletion set. We use a while loop here because the
-        // deletion set may grow as we call `clearDecl` within this loop,
-        // and more unreferenced Decls are revealed.
-        while (module.deletion_set.count() != 0) {
-            const decl_index = module.deletion_set.keys()[0];
-            const decl = module.declPtr(decl_index);
-            assert(decl.deletion_flag);
-            assert(decl.zir_decl_index != .none);
-
-            try module.clearDecl(decl_index, null);
-        }
-
         try module.processExports();
     }
 
@@ -3686,16 +3674,6 @@ pub fn performAllTheWork(
         try reportMultiModuleErrors(mod);
     }
 
-    {
-        const outdated_and_deleted_decls_frame = tracy.namedFrame("outdated_and_deleted_decls");
-        defer outdated_and_deleted_decls_frame.end();
-
-        // Iterate over all the files and look for outdated and deleted declarations.
-        if (comp.bin_file.options.module) |mod| {
-            try mod.processOutdatedAndDeletedDecls();
-        }
-    }
-
     if (comp.bin_file.options.module) |mod| {
         mod.sema_prog_node = main_progress_node.start("Semantic Analysis", 0);
         mod.sema_prog_node.activate();
src/InternPool.zig
@@ -6151,7 +6151,6 @@ fn finishFuncInstance(
         .@"linksection" = section,
         .@"addrspace" = fn_owner_decl.@"addrspace",
         .analysis = .complete,
-        .deletion_flag = false,
         .zir_decl_index = fn_owner_decl.zir_decl_index,
         .src_scope = fn_owner_decl.src_scope,
         .generation = generation,
@@ -7617,7 +7616,11 @@ pub fn createNamespace(
 }
 
 pub fn destroyNamespace(ip: *InternPool, gpa: Allocator, index: Module.Namespace.Index) void {
-    ip.namespacePtr(index).* = undefined;
+    ip.namespacePtr(index).* = .{
+        .parent = undefined,
+        .file_scope = undefined,
+        .ty = undefined,
+    };
     ip.namespaces_free_list.append(gpa, index) catch {
         // In order to keep `destroyNamespace` a non-fallible function, we ignore memory
         // allocation failures here, instead leaking the Namespace until garbage collection.
src/Module.zig
@@ -132,10 +132,6 @@ failed_exports: std.AutoArrayHashMapUnmanaged(*Export, *ErrorMsg) = .{},
 /// are stored here.
 cimport_errors: std.AutoArrayHashMapUnmanaged(Decl.Index, std.zig.ErrorBundle) = .{},
 
-/// Candidates for deletion. After a semantic analysis update completes, this list
-/// contains Decls that need to be deleted if they end up having no references to them.
-deletion_set: std.AutoArrayHashMapUnmanaged(Decl.Index, void) = .{},
-
 /// Key is the error name, index is the error tag value. Index 0 has a length-0 string.
 global_error_set: GlobalErrorSet = .{},
 
@@ -165,7 +161,7 @@ emit_h: ?*GlobalEmitH,
 
 test_functions: std.AutoArrayHashMapUnmanaged(Decl.Index, void) = .{},
 
-global_assembly: std.AutoHashMapUnmanaged(Decl.Index, []u8) = .{},
+global_assembly: std.AutoArrayHashMapUnmanaged(Decl.Index, []u8) = .{},
 
 reference_table: std.AutoHashMapUnmanaged(Decl.Index, struct {
     referencer: Decl.Index,
@@ -438,9 +434,6 @@ pub const Decl = struct {
     /// with it. That means when `Decl` is destroyed, the cleanup code should additionally
     /// check if the value owns a `Namespace`, and destroy that too.
     owns_tv: bool,
-    /// This flag is set when this Decl is added to `Module.deletion_set`, and cleared
-    /// when removed.
-    deletion_flag: bool,
     /// Whether the corresponding AST decl has a `pub` keyword.
     is_pub: bool,
     /// Whether the corresponding AST decl has a `export` keyword.
@@ -873,47 +866,6 @@ pub const Namespace = struct {
         }
     };
 
-    pub fn deinit(ns: *Namespace, mod: *Module) void {
-        ns.destroyDecls(mod);
-        ns.* = undefined;
-    }
-
-    pub fn destroyDecls(ns: *Namespace, mod: *Module) void {
-        const gpa = mod.gpa;
-
-        var decls = ns.decls;
-        ns.decls = .{};
-
-        for (decls.keys()) |decl_index| {
-            mod.destroyDecl(decl_index);
-        }
-        decls.deinit(gpa);
-
-        ns.usingnamespace_set.deinit(gpa);
-    }
-
-    pub fn deleteAllDecls(
-        ns: *Namespace,
-        mod: *Module,
-        outdated_decls: ?*std.AutoArrayHashMap(Decl.Index, void),
-    ) !void {
-        const gpa = mod.gpa;
-
-        var decls = ns.decls;
-        ns.decls = .{};
-
-        // TODO rework this code to not panic on OOM.
-        // (might want to coordinate with the clearDecl function)
-
-        for (decls.keys()) |child_decl| {
-            mod.clearDecl(child_decl, outdated_decls) catch @panic("out of memory");
-            mod.destroyDecl(child_decl);
-        }
-        decls.deinit(gpa);
-
-        ns.usingnamespace_set.deinit(gpa);
-    }
-
     // This renders e.g. "std.fs.Dir.OpenOptions"
     pub fn renderFullyQualifiedName(
         ns: Namespace,
@@ -2527,7 +2479,6 @@ pub fn deinit(mod: *Module) void {
         mod.embed_table.deinit(gpa);
     }
 
-    mod.deletion_set.deinit(gpa);
     mod.compile_log_text.deinit(gpa);
 
     mod.zig_cache_artifact_directory.handle.close();
@@ -2590,9 +2541,21 @@ pub fn deinit(mod: *Module) void {
 
     mod.test_functions.deinit(gpa);
 
+    for (mod.global_assembly.values()) |s| {
+        gpa.free(s);
+    }
     mod.global_assembly.deinit(gpa);
+
     mod.reference_table.deinit(gpa);
 
+    {
+        var it = mod.intern_pool.allocated_namespaces.iterator(0);
+        while (it.next()) |namespace| {
+            namespace.decls.deinit(gpa);
+            namespace.usingnamespace_set.deinit(gpa);
+        }
+    }
+
     mod.intern_pool.deinit(gpa);
     mod.tmp_hack_arena.deinit();
 
@@ -2606,20 +2569,10 @@ pub fn destroyDecl(mod: *Module, decl_index: Decl.Index) void {
     const ip = &mod.intern_pool;
 
     {
-        const decl = mod.declPtr(decl_index);
         _ = mod.test_functions.swapRemove(decl_index);
-        if (decl.deletion_flag) {
-            assert(mod.deletion_set.swapRemove(decl_index));
-        }
-        if (mod.global_assembly.fetchRemove(decl_index)) |kv| {
+        if (mod.global_assembly.fetchSwapRemove(decl_index)) |kv| {
             gpa.free(kv.value);
         }
-        if (decl.has_tv) {
-            if (decl.getOwnedInnerNamespaceIndex(mod).unwrap()) |i| {
-                mod.namespacePtr(i).destroyDecls(mod);
-                mod.destroyNamespace(i);
-            }
-        }
     }
 
     ip.destroyDecl(gpa, decl_index);
@@ -4422,56 +4375,6 @@ fn scanDecl(iter: *ScanDeclIter, decl_sub_index: usize, flags: u4) Allocator.Err
     }
 }
 
-/// Make it as if the semantic analysis for this Decl never happened.
-pub fn clearDecl(
-    mod: *Module,
-    decl_index: Decl.Index,
-    outdated_decls: ?*std.AutoArrayHashMap(Decl.Index, void),
-) Allocator.Error!void {
-    const tracy = trace(@src());
-    defer tracy.end();
-
-    const decl = mod.declPtr(decl_index);
-
-    const gpa = mod.gpa;
-
-    if (outdated_decls) |map| {
-        _ = map.swapRemove(decl_index);
-    }
-
-    if (mod.failed_decls.fetchSwapRemove(decl_index)) |kv| {
-        kv.value.destroy(gpa);
-    }
-    if (mod.cimport_errors.fetchSwapRemove(decl_index)) |kv| {
-        var errors = kv.value;
-        errors.deinit(gpa);
-    }
-    if (mod.emit_h) |emit_h| {
-        if (emit_h.failed_decls.fetchSwapRemove(decl_index)) |kv| {
-            kv.value.destroy(gpa);
-        }
-        assert(emit_h.decl_table.swapRemove(decl_index));
-    }
-    _ = mod.compile_log_decls.swapRemove(decl_index);
-    try mod.deleteDeclExports(decl_index);
-
-    if (decl.has_tv) {
-        if (decl.ty.isFnOrHasRuntimeBits(mod)) {
-            mod.comp.bin_file.freeDecl(decl_index);
-        }
-        if (decl.getOwnedInnerNamespace(mod)) |namespace| {
-            try namespace.deleteAllDecls(mod, outdated_decls);
-        }
-    }
-
-    if (decl.deletion_flag) {
-        decl.deletion_flag = false;
-        assert(mod.deletion_set.swapRemove(decl_index));
-    }
-
-    decl.analysis = .unreferenced;
-}
-
 /// This function is exclusively called for anonymous decls.
 /// All resources referenced by anonymous decls are owned by InternPool
 /// so there is no cleanup to do here.
@@ -4488,14 +4391,6 @@ pub fn deleteUnusedDecl(mod: *Module, decl_index: Decl.Index) void {
     }
 }
 
-/// We don't perform a deletion here, because this Decl or another one
-/// may end up referencing it before the update is complete.
-fn markDeclForDeletion(mod: *Module, decl_index: Decl.Index) !void {
-    const decl = mod.declPtr(decl_index);
-    decl.deletion_flag = true;
-    try mod.deletion_set.put(mod.gpa, decl_index, {});
-}
-
 /// Cancel the creation of an anon decl and delete any references to it.
 /// If other decls depend on this decl, they must be aborted first.
 pub fn abortAnonDecl(mod: *Module, decl_index: Decl.Index) void {
@@ -4868,7 +4763,6 @@ pub fn allocateNewDecl(
         .@"linksection" = .none,
         .@"addrspace" = .generic,
         .analysis = .unreferenced,
-        .deletion_flag = false,
         .zir_decl_index = .none,
         .src_scope = src_scope,
         .generation = 0,
@@ -5366,52 +5260,6 @@ pub fn optionsSrc(mod: *Module, decl: *Decl, base_src: LazySrcLoc, wanted: []con
     return base_src;
 }
 
-/// Called from `performAllTheWork`, after all AstGen workers have finished,
-/// and before the main semantic analysis loop begins.
-pub fn processOutdatedAndDeletedDecls(mod: *Module) !void {
-    // Ultimately, the goal is to queue up `analyze_decl` tasks in the work queue
-    // for the outdated decls, but we cannot queue up the tasks until after
-    // we find out which ones have been deleted, otherwise there would be
-    // deleted Decl pointers in the work queue.
-    var outdated_decls = std.AutoArrayHashMap(Decl.Index, void).init(mod.gpa);
-    defer outdated_decls.deinit();
-    for (mod.import_table.values()) |file| {
-        try outdated_decls.ensureUnusedCapacity(file.outdated_decls.items.len);
-        for (file.outdated_decls.items) |decl_index| {
-            outdated_decls.putAssumeCapacity(decl_index, {});
-        }
-        file.outdated_decls.clearRetainingCapacity();
-
-        // Handle explicitly deleted decls from the source code. This is one of two
-        // places that Decl deletions happen. The other is in `Compilation`, after
-        // `performAllTheWork`, where we iterate over `Module.deletion_set` and
-        // delete Decls which are no longer referenced.
-        // If a Decl is explicitly deleted from source, and also no longer referenced,
-        // it may be both in this `deleted_decls` set, as well as in the
-        // `Module.deletion_set`. To avoid deleting it twice, we remove it from the
-        // deletion set at this time.
-        for (file.deleted_decls.items) |decl_index| {
-            const decl = mod.declPtr(decl_index);
-
-            // Remove from the namespace it resides in, preserving declaration order.
-            assert(decl.zir_decl_index != .none);
-            _ = mod.namespacePtr(decl.src_namespace).decls.orderedRemoveAdapted(
-                decl.name,
-                DeclAdapter{ .mod = mod },
-            );
-
-            try mod.clearDecl(decl_index, &outdated_decls);
-            mod.destroyDecl(decl_index);
-        }
-        file.deleted_decls.clearRetainingCapacity();
-    }
-    // Finally we can queue up re-analysis tasks after we have processed
-    // the deleted decls.
-    for (outdated_decls.keys()) |key| {
-        try mod.markOutdatedDecl(key);
-    }
-}
-
 /// Called from `Compilation.update`, after everything is done, just before
 /// reporting compile errors. In this function we emit exported symbol collision
 /// errors and communicate exported symbols to the linker backend.