From 46388d338a93a35d139866411f80115a03b30a6a Mon Sep 17 00:00:00 2001 From: mlugg Date: Wed, 14 Aug 2024 08:10:49 +0100 Subject: [PATCH] InternPool: don't remove outdated types When a type becomes outdated, there will still be lingering references to the old index -- for instance, any declaration whose value was that type holds a reference to that index. These references may live for an arbitrarily long time in some cases. So, we can't just remove the type from the pool -- the old `Index` must remain valid! Instead, we want to preserve the old `Index`, but avoid it from ever appearing in lookups. (It's okay if analysis of something referencing the old `Index` does weird stuff -- such analysis are guaranteed by the incremental compilation model to always be unreferenced.) So, we use the new `InternPool.putKeyReplace` to replace the shard entry for this index with the newly-created index. --- src/InternPool.zig | 71 +++++++++++++++++++++++++++++++++++++++---- src/Sema.zig | 33 ++++++++++---------- src/Zcu/PerThread.zig | 15 ++++----- 3 files changed, 87 insertions(+), 32 deletions(-) diff --git a/src/InternPool.zig b/src/InternPool.zig index 91a58e10e7..7c1b37d3d4 100644 --- a/src/InternPool.zig +++ b/src/InternPool.zig @@ -7077,6 +7077,7 @@ fn getOrPutKeyEnsuringAdditionalCapacity( const index = entry.acquire(); if (index == .none) break; if (entry.hash != hash) continue; + if (ip.isRemoved(index)) continue; if (ip.indexToKey(index).eql(key, ip)) return .{ .existing = index }; } shard.mutate.map.mutex.lock(); @@ -7151,6 +7152,43 @@ fn getOrPutKeyEnsuringAdditionalCapacity( .map_index = map_index, } }; } +/// Like `getOrPutKey`, but asserts that the key already exists, and prepares to replace +/// its shard entry with a new `Index` anyway. After finalizing this, the old index remains +/// valid (in that `indexToKey` and similar queries will behave as before), but it will +/// never be returned from a lookup (`getOrPutKey` etc). +/// This is used by incremental compilation when an existing container type is outdated. In +/// this case, the type must be recreated at a new `InternPool.Index`, but the old index must +/// remain valid since now-unreferenced `AnalUnit`s may retain references to it. The old index +/// will be cleaned up when the `Zcu` undergoes garbage collection. +fn putKeyReplace( + ip: *InternPool, + tid: Zcu.PerThread.Id, + key: Key, +) GetOrPutKey { + const full_hash = key.hash64(ip); + const hash: u32 = @truncate(full_hash >> 32); + const shard = &ip.shards[@intCast(full_hash & (ip.shards.len - 1))]; + shard.mutate.map.mutex.lock(); + errdefer shard.mutate.map.mutex.unlock(); + const map = shard.shared.map; + const map_mask = map.header().mask(); + var map_index = hash; + while (true) : (map_index += 1) { + map_index &= map_mask; + const entry = &map.entries[map_index]; + const index = entry.value; + assert(index != .none); // key not present + if (entry.hash == hash and ip.indexToKey(index).eql(key, ip)) { + break; // we found the entry to replace + } + } + return .{ .new = .{ + .ip = ip, + .tid = tid, + .shard = shard, + .map_index = map_index, + } }; +} pub fn get(ip: *InternPool, gpa: Allocator, tid: Zcu.PerThread.Id, key: Key) Allocator.Error!Index { var gop = try ip.getOrPutKey(gpa, tid, key); @@ -7990,8 +8028,11 @@ pub fn getUnionType( gpa: Allocator, tid: Zcu.PerThread.Id, ini: UnionTypeInit, + /// If it is known that there is an existing type with this key which is outdated, + /// this is passed as `true`, and the type is replaced with one at a fresh index. + replace_existing: bool, ) Allocator.Error!WipNamespaceType.Result { - var gop = try ip.getOrPutKey(gpa, tid, .{ .union_type = switch (ini.key) { + const key: Key = .{ .union_type = switch (ini.key) { .declared => |d| .{ .declared = .{ .zir_index = d.zir_index, .captures = .{ .external = d.captures }, @@ -8000,7 +8041,11 @@ pub fn getUnionType( .zir_index = r.zir_index, .type_hash = r.type_hash, } }, - } }); + } }; + var gop = if (replace_existing) + ip.putKeyReplace(tid, key) + else + try ip.getOrPutKey(gpa, tid, key); defer gop.deinit(); if (gop == .existing) return .{ .existing = gop.existing }; @@ -8166,8 +8211,11 @@ pub fn getStructType( gpa: Allocator, tid: Zcu.PerThread.Id, ini: StructTypeInit, + /// If it is known that there is an existing type with this key which is outdated, + /// this is passed as `true`, and the type is replaced with one at a fresh index. + replace_existing: bool, ) Allocator.Error!WipNamespaceType.Result { - var gop = try ip.getOrPutKey(gpa, tid, .{ .struct_type = switch (ini.key) { + const key: Key = .{ .struct_type = switch (ini.key) { .declared => |d| .{ .declared = .{ .zir_index = d.zir_index, .captures = .{ .external = d.captures }, @@ -8176,7 +8224,11 @@ pub fn getStructType( .zir_index = r.zir_index, .type_hash = r.type_hash, } }, - } }); + } }; + var gop = if (replace_existing) + ip.putKeyReplace(tid, key) + else + try ip.getOrPutKey(gpa, tid, key); defer gop.deinit(); if (gop == .existing) return .{ .existing = gop.existing }; @@ -9200,8 +9252,11 @@ pub fn getEnumType( gpa: Allocator, tid: Zcu.PerThread.Id, ini: EnumTypeInit, + /// If it is known that there is an existing type with this key which is outdated, + /// this is passed as `true`, and the type is replaced with one at a fresh index. + replace_existing: bool, ) Allocator.Error!WipEnumType.Result { - var gop = try ip.getOrPutKey(gpa, tid, .{ .enum_type = switch (ini.key) { + const key: Key = .{ .enum_type = switch (ini.key) { .declared => |d| .{ .declared = .{ .zir_index = d.zir_index, .captures = .{ .external = d.captures }, @@ -9210,7 +9265,11 @@ pub fn getEnumType( .zir_index = r.zir_index, .type_hash = r.type_hash, } }, - } }); + } }; + var gop = if (replace_existing) + ip.putKeyReplace(tid, key) + else + try ip.getOrPutKey(gpa, tid, key); defer gop.deinit(); if (gop == .existing) return .{ .existing = gop.existing }; diff --git a/src/Sema.zig b/src/Sema.zig index 1aedd745ea..d760927c4d 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -2724,9 +2724,9 @@ fn wrapWipTy(sema: *Sema, wip_ty: anytype) @TypeOf(wip_ty) { } /// Given a type just looked up in the `InternPool`, check whether it is -/// considered outdated on this update. If so, remove it from the pool -/// and return `true`. -fn maybeRemoveOutdatedType(sema: *Sema, ty: InternPool.Index) !bool { +/// considered outdated on this update. If so, returns `true`, and the +/// caller must replace the outdated type with a fresh one. +fn checkOutdatedType(sema: *Sema, ty: InternPool.Index) !bool { const pt = sema.pt; const zcu = pt.zcu; const ip = &zcu.intern_pool; @@ -2745,7 +2745,6 @@ fn maybeRemoveOutdatedType(sema: *Sema, ty: InternPool.Index) !bool { if (!was_outdated) return false; _ = zcu.outdated_ready.swapRemove(cau_unit); zcu.intern_pool.removeDependenciesForDepender(zcu.gpa, cau_unit); - zcu.intern_pool.remove(pt.tid, ty); try zcu.markDependeeOutdated(.marked_po, .{ .interned = ty }); return true; } @@ -2815,14 +2814,14 @@ fn zirStructDecl( .captures = captures, } }, }; - const wip_ty = sema.wrapWipTy(switch (try ip.getStructType(gpa, pt.tid, struct_init)) { + const wip_ty = sema.wrapWipTy(switch (try ip.getStructType(gpa, pt.tid, struct_init, false)) { .existing => |ty| wip: { - if (!try sema.maybeRemoveOutdatedType(ty)) { + if (!try sema.checkOutdatedType(ty)) { try sema.declareDependency(.{ .interned = ty }); try sema.addTypeReferenceEntry(src, ty); return Air.internedToRef(ty); } - break :wip (try ip.getStructType(gpa, pt.tid, struct_init)).wip; + break :wip (try ip.getStructType(gpa, pt.tid, struct_init, true)).wip; }, .wip => |wip| wip, }); @@ -3041,14 +3040,14 @@ fn zirEnumDecl( .captures = captures, } }, }; - const wip_ty = sema.wrapWipTy(switch (try ip.getEnumType(gpa, pt.tid, enum_init)) { + const wip_ty = sema.wrapWipTy(switch (try ip.getEnumType(gpa, pt.tid, enum_init, false)) { .existing => |ty| wip: { - if (!try sema.maybeRemoveOutdatedType(ty)) { + if (!try sema.checkOutdatedType(ty)) { try sema.declareDependency(.{ .interned = ty }); try sema.addTypeReferenceEntry(src, ty); return Air.internedToRef(ty); } - break :wip (try ip.getEnumType(gpa, pt.tid, enum_init)).wip; + break :wip (try ip.getEnumType(gpa, pt.tid, enum_init, true)).wip; }, .wip => |wip| wip, }); @@ -3311,14 +3310,14 @@ fn zirUnionDecl( .captures = captures, } }, }; - const wip_ty = sema.wrapWipTy(switch (try ip.getUnionType(gpa, pt.tid, union_init)) { + const wip_ty = sema.wrapWipTy(switch (try ip.getUnionType(gpa, pt.tid, union_init, false)) { .existing => |ty| wip: { - if (!try sema.maybeRemoveOutdatedType(ty)) { + if (!try sema.checkOutdatedType(ty)) { try sema.declareDependency(.{ .interned = ty }); try sema.addTypeReferenceEntry(src, ty); return Air.internedToRef(ty); } - break :wip (try ip.getUnionType(gpa, pt.tid, union_init)).wip; + break :wip (try ip.getUnionType(gpa, pt.tid, union_init, true)).wip; }, .wip => |wip| wip, }); @@ -3407,7 +3406,7 @@ fn zirOpaqueDecl( }; // No `wrapWipTy` needed as no std.builtin types are opaque. const wip_ty = switch (try ip.getOpaqueType(gpa, pt.tid, opaque_init)) { - // No `maybeRemoveOutdatedType` as opaque types are never outdated. + // No `checkOutdatedType` as opaque types are never outdated. .existing => |ty| { try sema.addTypeReferenceEntry(src, ty); return Air.internedToRef(ty); @@ -22054,7 +22053,7 @@ fn reifyEnum( .zir_index = tracked_inst, .type_hash = hasher.final(), } }, - })) { + }, false)) { .wip => |wip| wip, .existing => |ty| { try sema.declareDependency(.{ .interned = ty }); @@ -22224,7 +22223,7 @@ fn reifyUnion( .zir_index = tracked_inst, .type_hash = hasher.final(), } }, - })) { + }, false)) { .wip => |wip| wip, .existing => |ty| { try sema.declareDependency(.{ .interned = ty }); @@ -22494,7 +22493,7 @@ fn reifyStruct( .zir_index = tracked_inst, .type_hash = hasher.final(), } }, - })) { + }, false)) { .wip => |wip| wip, .existing => |ty| { try sema.declareDependency(.{ .interned = ty }); diff --git a/src/Zcu/PerThread.zig b/src/Zcu/PerThread.zig index 2720edd2f2..83a7dce4fc 100644 --- a/src/Zcu/PerThread.zig +++ b/src/Zcu/PerThread.zig @@ -925,6 +925,7 @@ fn createFileRootStruct( pt: Zcu.PerThread, file_index: Zcu.File.Index, namespace_index: Zcu.Namespace.Index, + replace_existing: bool, ) Allocator.Error!InternPool.Index { const zcu = pt.zcu; const gpa = zcu.gpa; @@ -968,7 +969,7 @@ fn createFileRootStruct( .zir_index = tracked_inst, .captures = &.{}, } }, - })) { + }, replace_existing)) { .existing => unreachable, // we wouldn't be analysing the file root if this type existed .wip => |wip| wip, }; @@ -1023,8 +1024,7 @@ fn recreateFileRoot(pt: Zcu.PerThread, file_index: Zcu.File.Index) Zcu.SemaError zcu.gpa, InternPool.AnalUnit.wrap(.{ .cau = file_root_type_cau }), ); - ip.remove(pt.tid, file_root_type); - _ = try pt.createFileRootStruct(file_index, namespace_index); + _ = try pt.createFileRootStruct(file_index, namespace_index, true); } /// Re-scan the namespace of a file's root struct type on an incremental update. @@ -1062,8 +1062,6 @@ fn updateFileNamespace(pt: Zcu.PerThread, file_index: Zcu.File.Index) Allocator. try pt.scanNamespace(namespace_index, decls); } -/// Regardless of the file status, will create a `Decl` if none exists so that we can track -/// dependencies and re-analyze when the file becomes outdated. fn semaFile(pt: Zcu.PerThread, file_index: Zcu.File.Index) Zcu.SemaError!void { const tracy = trace(@src()); defer tracy.end(); @@ -1083,7 +1081,7 @@ fn semaFile(pt: Zcu.PerThread, file_index: Zcu.File.Index) Zcu.SemaError!void { .owner_type = undefined, // set in `createFileRootStruct` .file_scope = file_index, }); - const struct_ty = try pt.createFileRootStruct(file_index, new_namespace_index); + const struct_ty = try pt.createFileRootStruct(file_index, new_namespace_index, false); errdefer zcu.intern_pool.remove(pt.tid, struct_ty); switch (zcu.comp.cache_use) { @@ -1153,11 +1151,10 @@ fn semaCau(pt: Zcu.PerThread, cau_index: InternPool.Cau.Index) !SemaCauResult { // This declaration has no value so is definitely not a std.builtin type. break :ip_index .none; }, - .type => |ty| { + .type => { // This is an incremental update, and this type is being re-analyzed because it is outdated. // The type must be recreated at a new `InternPool.Index`. - // Remove it from the InternPool and mark it outdated so that creation sites are re-analyzed. - ip.remove(pt.tid, ty); + // Mark it outdated so that creation sites are re-analyzed. return .{ .invalidate_decl_val = true, .invalidate_decl_ref = true,