Skip to content

macho: add preliminary non-prealloc path to the linker #10280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 6, 2021

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented Dec 5, 2021

The non-prealloc path is currently enabled for stage1 by default, and in the future, it might also apply to release builds if we decide not to have incremental updates then. What it means is that, when the linker uses this codepath, it doesn't preallocate segment and section sizes, much like it's done in any traditional linker.

The reason I reintroduced this is to minimize the binary size blow up that many have observed when building with stage1. With this PR, we now get the following binary size improvements (linking speed is largely the same for both codepaths):

(everything measured on my M1 MBA)

  • stage2 (with -Dlog -Denable-llvm -Dlink-snapshot flags)
    • 17.5% size reduction from 237.8MB down to 196.1MB
  • multi-file small C program
    • 86.8% size reduction from 378KB down to 49KB
  • Andrew's zig-sdl library
    • 17.4% size reduction from 292.6MB down to 241.6MB

Fixes #9990

@andrewrk
Copy link
Member

andrewrk commented Dec 5, 2021

Looks like this addresses #9990. Would you say it resolves it?

@kubkon
Copy link
Member Author

kubkon commented Dec 5, 2021

Looks like this addresses #9990. Would you say it resolves it?

Nice one, well spotted! Let me link this issue as resolved by this PR. Thanks Andrew!

@andrewrk
Copy link
Member

andrewrk commented Dec 6, 2021

Possibly relevant to this, in a local branch I have this diff:

--- a/src/Compilation.zig
+++ b/src/Compilation.zig
@@ -69,6 +69,8 @@ failed_c_objects: std.AutoArrayHashMapUnmanaged(*CObject, *CObject.ErrorMsg) = .
 /// Miscellaneous things that can fail.
 misc_failures: std.AutoArrayHashMapUnmanaged(MiscTask, MiscError) = .{},
 
+cache_mode: CacheMode,
+
 keep_source_files_loaded: bool,
 use_clang: bool,
 sanitize_c: bool,
@@ -632,6 +634,8 @@ pub const ClangPreprocessorMode = enum {
 
 pub const SystemLib = link.SystemLib;
 
+pub const CacheMode = enum { incremental, whole };
+
 pub const InitOptions = struct {
     zig_lib_directory: Directory,
     local_cache_directory: Directory,
@@ -668,6 +672,7 @@ pub const InitOptions = struct {
     /// is externally modified - essentially anything other than zig-cache - then
     /// this flag would be set to disable this machinery to avoid false positives.
     disable_lld_caching: bool = false,
+    cache_mode: CacheMode = .incremental,
     object_format: ?std.Target.ObjectFormat = null,
     optimize_mode: std.builtin.Mode = .Debug,
     keep_source_files_loaded: bool = false,

This is actually part of a bug fix to #9439, as well as a compilation speed performance enhancement - idea being that for some compilations we actually want to opt out of incremental compilation and just cache the whole frickin thing. For example, there is no sense in incrementally building compiler-rt, which we build using the LLVM backend, using -OReleaseFast. With incremental compilation enabled, every compilation is waiting for LLVM to optimize the exact same compiler-rt LLVM module. Same with libc and other commonly shared files.

Anyway point being, I wanted to alert you to this upcoming addition because I get the sense that the linker code may benefit from observing this cache_mode setting.

@kubkon
Copy link
Member Author

kubkon commented Dec 6, 2021

Possibly relevant to this, in a local branch I have this diff:

--- a/src/Compilation.zig
+++ b/src/Compilation.zig
@@ -69,6 +69,8 @@ failed_c_objects: std.AutoArrayHashMapUnmanaged(*CObject, *CObject.ErrorMsg) = .
 /// Miscellaneous things that can fail.
 misc_failures: std.AutoArrayHashMapUnmanaged(MiscTask, MiscError) = .{},
 
+cache_mode: CacheMode,
+
 keep_source_files_loaded: bool,
 use_clang: bool,
 sanitize_c: bool,
@@ -632,6 +634,8 @@ pub const ClangPreprocessorMode = enum {
 
 pub const SystemLib = link.SystemLib;
 
+pub const CacheMode = enum { incremental, whole };
+
 pub const InitOptions = struct {
     zig_lib_directory: Directory,
     local_cache_directory: Directory,
@@ -668,6 +672,7 @@ pub const InitOptions = struct {
     /// is externally modified - essentially anything other than zig-cache - then
     /// this flag would be set to disable this machinery to avoid false positives.
     disable_lld_caching: bool = false,
+    cache_mode: CacheMode = .incremental,
     object_format: ?std.Target.ObjectFormat = null,
     optimize_mode: std.builtin.Mode = .Debug,
     keep_source_files_loaded: bool = false,

This is actually part of a bug fix to #9439, as well as a compilation speed performance enhancement - idea being that for some compilations we actually want to opt out of incremental compilation and just cache the whole frickin thing. For example, there is no sense in incrementally building compiler-rt, which we build using the LLVM backend, using -OReleaseFast. With incremental compilation enabled, every compilation is waiting for LLVM to optimize the exact same compiler-rt LLVM module. Same with libc and other commonly shared files.

Anyway point being, I wanted to alert you to this upcoming addition because I get the sense that the linker code may benefit from observing this cache_mode setting.

Nice! That's going to be very relevant for disambiguating the two code paths in stage2 I think :-)

@kubkon
Copy link
Member Author

kubkon commented Dec 6, 2021

Hmm, I don't think that failing aarch64 drone CI is my fault so I'm going to merge and if the failure persists on master, I'll try investigating what's up.

@kubkon kubkon merged commit 933999d into master Dec 6, 2021
@kubkon kubkon deleted the zld-rel-code-path branch December 6, 2021 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

larger than expected macOS binaries
2 participants