enhancement(releasing): change release profile #6202
Conversation
fanatid
force-pushed
the
binary-size
branch
3 times, most recently
from
c4654e4 to
236103f
Compare
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
fanatid
changed the title
There was a problem hiding this comment.
Nice work @fanatid! I appreciate the table you laid out here.
I'm actually a little surprised that we are stripping debug info via link-arg=-s as it makes crash reports much less useful. I see this was added in 648e82c but I don't see a specific reason given for it.
My thoughts are:
For now we go with opt-level 3, lto, codegen_units=1, panic=unwind, and keep debug symbols in. That still seems to give us a 50% improvement on binary size without losing debug information. The issue this addresses, #6202, seems to just be looking for quick wins and I think that set of flags would give it to us.
We create a separate issue for separating debug symbols from the release binaries as part of the release builds so that we can hold on to them for debugging purposes and to correlate stack traces if users report them. This seems like it will give us another ~50% boon. We can investigate abort vs. unwind then. I've never actually done this, but it seems to be possible to separate the symbols via objcopy followed by a strip to remove the symbols.
The alternative is that we keep going without debug symbols and ask people to run a debug build if they report a crash. I'm open to this as well, but would want to make a conscious choice in light of the trade-offs.
I think people seriously concerned about binary size may also be willing to go the extra mile of a conditional build of vector with exactly the features they use.
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
|
Sounds good, I removed flags for removing debug symbols and changed Checked size again: (shared settings:
So if somebody wants super slim size they should consider compiling |
It's not clear to me why profile.dev was being written to Cargo.toml by slim-builds.sh given that all of the values are defaulted. The one value, `incremental`, that is changed, is changed in `~/.cargo/config` which will override any profiles. > The incremental value can be overridden globally with the CARGO_INCREMENTAL environment variable or the build.incremental config variable. https://doc.rust-lang.org/cargo/reference/profiles.html#incremental I think the release profile is similar, but will be removed by #6202 so I left it for now. Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
It's not clear to me why profile.dev was being written to Cargo.toml by slim-builds.sh given that all of the values are defaulted. The one value, `incremental`, that is changed, is changed in `~/.cargo/config` which will override any profiles. > The incremental value can be overridden globally with the CARGO_INCREMENTAL environment variable or the build.incremental config variable. https://doc.rust-lang.org/cargo/reference/profiles.html#incremental I think the release profile is similar, but will be removed by #6202 so I left it for now. Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
| # See defaults https://doc.rust-lang.org/cargo/reference/profiles.html#release | ||
| opt-level = 3 | ||
| debug = false | ||
| debug-assertions = false | ||
| overflow-checks = false | ||
| lto = false | ||
| panic = 'unwind' | ||
| # Disabled, see build.incremental | ||
| # incremental = false | ||
| codegen-units = 1 | ||
| rpath = false |
There was a problem hiding this comment.
In the comment it was noted these measures were added because we hit disk size limits on CI. Is that not a problem anymore (or, codegen-units and lto trim enough fat)?
There was a problem hiding this comment.
Not sure is the disk size limit on CI still an issue or not (@jszwedko ?). I just tested default release profile (lto = false, codegen-units = 16) vs PR profile (lto = true, codegen-units = 1).
default: 2_782_000 kb
PR: 2_869_444 kb
There was a problem hiding this comment.
We can see 😄 The release builds still happen on Github Actions runners so they may still hit disk limits.
There was a problem hiding this comment.
Again, I think the only real change here is lto = true. We were already building with codgen-units = 1.
| @@ -16,6 +16,10 @@ name = "graphql-schema" | |||
| path = "src/api/schema/gen.rs" | |||
| required-features = ["default-no-api-client"] | |||
|
|
|||
| [profile.release] | |||
| lto = true | |||
| codegen-units = 1 | |||
There was a problem hiding this comment.
My only concern here is that we had codegen-units = 1 previously and decided to remove it because of how much longer it makes the build take (if I remember correctly).
There was a problem hiding this comment.
Yes, lto=true+codegen-units=1 require more time, but we do not need this too often.
There was a problem hiding this comment.
We were already using codegen-units = 1 previously. The only real change here, I think, is lto.
Close #5765
opt-levelltocodegen_unitsCFLAGS ("-g0")CFLAGS ("-O3")RUSTFLAGS ("-C link-arg=-s")panicsizesize (gzip)unwindunwindunwindunwindunwindunwindabortunwindunwindunwindunwindunwindabortunwindabortI think most relevant release config for us will be:
We can use
opt-level = "s" / "z"for smaller size but from guides this looks like affect performance (I tried collect benchmark, but CI fail forlto = true / codegen-units = 1).Since we distribute
vectorbinary without debug information (because we useRUSTFLAGS="-C link-arg=-s") I think we also can usepanic = "abort"(save ~4MB).I also added an optimization flag (
-O3) for C libraries throughCFLAGS(discover this when tried to remove debug information from there but then realized that we do not have it for the whole binary).`unwind` / `abort` backtraces
unwindabort