InternPool: begin conversion to thread-safe data structure#20528
Merged
andrewrk merged 15 commits intoziglang:masterfrom Jul 8, 2024
Merged
InternPool: begin conversion to thread-safe data structure#20528andrewrk merged 15 commits intoziglang:masterfrom
andrewrk merged 15 commits intoziglang:masterfrom
Conversation
This was just a badly implemented arena anyway.
This reduces the cost of the new data structure until the multi-threaded behavior is actually used.
This allows them to be atomically replaced.
(There are no supported backends.)
Member
|
Data point: building the zig compiler with the x86_64 backend with codegen threading enabled: --- a/src/InternPool.zig
+++ b/src/InternPool.zig
@@ -93,7 +93,7 @@ files: std.AutoArrayHashMapUnmanaged(Cache.BinDigest, OptionalDeclIndex) = .{},
/// Whether a multi-threaded intern pool is useful.
/// Currently `false` until the intern pool is actually accessed
/// from multiple threads to reduce the cost of this data structure.
-const want_multi_threaded = false;
+const want_multi_threaded = true;
/// Whether a single-threaded intern pool impl is in use.
pub const single_threaded = builtin.single_threaded or !want_multi_threaded;
diff --git a/src/target.zig b/src/target.zig
index 2accc100b8..e236cea616 100644
--- a/src/target.zig
+++ b/src/target.zig
@@ -572,6 +572,7 @@ pub inline fn backendSupportsFeature(backend: std.builtin.CompilerBackend, compt
else => false,
},
.separate_thread => switch (backend) {
+ .stage2_x86_64 => true,
else => false,
},
};before: 8f20e81 |
Contributor
Member
|
Note that my 12.8s data point above is with the x86_64 backend. You can try enabling it, but it's not the default yet due to machine code quality, debug info correctness, and behavior test coverage. Lines 218 to 220 in 854e86c If you're on new Apple hardware you'll have to wait for the aarch64 backend to get a similar speedup. |
40 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

This is another step towards first putting codegen on a separate thread from sema and later making both sema and codegen multi-threaded. Currently, the multi-threaded features of the new data structure are disabled by
InternPool.want_multi_threadedbecause they have a large performance impact on the compiler and are not yet in use. It is still undecided how to minimize the impact ofThe main conflicting change is replacing
*ZcuwithZcu.PerThreadin any code that wants to be able to mutate the intern pool. I chose to pass this information around everywhere instead of attempting something withthreadlocalvariables because in the fast case that would turn the intern pool into a global singleton, and in the slow case would introduce an extra lookup on every intern pool mutation. Interestingly, if there is a desire to remove intern pool mutation from the backends, the per thread change can be reverted over time in parts of the code and that guarantees that the mutations can no longer in that section.The current performance impact is in the range where I expect to be able to recoup the loss with more work, because while timing individual commits, this is close to the same time as before the change which ended up having the worst performance impact, which I mitigated by disabling features of the data structure as mentioned above. Even with this current slowdown, there may be a desire to get some or all of this change merged sooner to reduce future conflicts, and continue work separately.