Skip to content
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.

Sign up for GitHub

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

enhancement(releasing): change release profile #6202

Merged
merged 6 commits into from
Feb 9, 2021
Merged

enhancement(releasing): change release profile #6202

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6c6f7ee
enhancement: use codegen_units/lto for release
fanatid Jan 24, 2021
aa4aab5
set panic to abort
fanatid Jan 25, 2021
d9a545e
remove rustflags, change panic
fanatid Feb 8, 2021
1c71128
Merge remote-tracking branch 'upstream/master' into binary-size
fanatid Feb 8, 2021
c2286ac
remove KEEP_SYMBOLS from build.sh
fanatid Feb 9, 2021
c87237d
Merge remote-tracking branch 'upstream/master' into binary-size
fanatid Feb 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ name = "graphql-schema"
path = "src/api/schema/gen.rs"
required-features = ["default-no-api-client"]

[profile.release]
lto = true
panic = "abort"
codegen-units = 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, lto=true+codegen-units=1 require more time, but we do not need this too often.

Copy link
Member

@jszwedko jszwedko Feb 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were already using codegen-units = 1 previously. The only real change here, I think, is lto.

https://github.com/timberio/vector/pull/6202/files#diff-e5855d7cc5d4b3fdd03bd1cf1291d5bb15017d416fdd822ec7eb735444cd20b5L34


[profile.bench]
debug = true

Expand Down
6 changes: 6 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ environment-push: environment-prepare ## Publish a new version of the container

##@ Building
.PHONY: build
build: export RUSTFLAGS += -C link-arg=-s
build: export CFLAGS += -g0 -O3
build: ## Build the project in release mode (Supports `ENVIRONMENT=true`)
${MAYBE_ENVIRONMENT_EXEC} cargo build --release --no-default-features --features ${DEFAULT_FEATURES}
${MAYBE_ENVIRONMENT_COPY_ARTIFACTS}
Expand Down Expand Up @@ -215,6 +217,7 @@ cross-%: export COMMAND ?=$(word 1,${PAIR})
cross-%: export TRIPLE ?=$(subst ${SPACE},-,$(wordlist 2,99,${PAIR}))
cross-%: export PROFILE ?= release
cross-%: export RUSTFLAGS += -C link-arg=-s
cross-%: export CFLAGS += -g0 -O3
cross-%: cargo-install-cross
$(MAKE) -k cross-image-${TRIPLE}
cross ${COMMAND} \
Expand All @@ -227,6 +230,7 @@ target/%/vector: export PAIR =$(subst /, ,$(@:target/%/vector=%))
target/%/vector: export TRIPLE ?=$(word 1,${PAIR})
target/%/vector: export PROFILE ?=$(word 2,${PAIR})
target/%/vector: export RUSTFLAGS += -C link-arg=-s
target/%/vector: export CFLAGS += -g0 -O3
target/%/vector: cargo-install-cross CARGO_HANDLES_FRESHNESS
$(MAKE) -k cross-image-${TRIPLE}
cross build \
Expand Down Expand Up @@ -515,6 +519,8 @@ test-cli: ## Runs cli tests
test-wasm-build-modules: $(WASM_MODULE_OUTPUTS) ### Build all WASM test modules

$(WASM_MODULE_OUTPUTS): MODULE = $(notdir $@)
$(WASM_MODULE_OUTPUTS): export RUSTFLAGS += -C link-arg=-s
$(WASM_MODULE_OUTPUTS): export CFLAGS += -g0 -O3
$(WASM_MODULE_OUTPUTS): ### Build a specific WASM module
@echo "# Building WASM module ${MODULE}, requires Rustc for wasm32-wasi."
${MAYBE_ENVIRONMENT_EXEC} cargo build \
Expand Down
22 changes: 14 additions & 8 deletions scripts/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,20 @@ if [ -f "$BINARY_PATH" ] && [ "$OVERWRITE" == "false" ]; then
exit 0
fi

#
# CFLAGS
#

export CFLAGS="$CFLAGS -g0 -O3"

#
# Strip the output binary through RUSTFLAGS
#

if [ "$KEEP_SYMBOLS" != "false" ]; then
export RUSTFLAGS="$RUSTFLAGS -C link-arg=-s"
fi

#
# Header
#
Expand Down Expand Up @@ -84,11 +98,3 @@ if [ "$FEATURES" == "default" ]; then
else
cargo build "${BUILD_FLAGS[@]}" --no-default-features --features "$FEATURES"
fi

#
# Strip the output binary
#

if [ "$KEEP_SYMBOLS" == "false" ]; then
strip "$BINARY_PATH"
fi
13 changes: 0 additions & 13 deletions scripts/slim-builds.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,4 @@ panic = 'unwind'
# incremental = true
codegen-units = 256
rpath = false

[profile.release]
# 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
Comment on lines -25 to -35
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can see 😄 The release builds still happen on Github Actions runners so they may still hit disk limits.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I think the only real change here is lto = true. We were already building with codgen-units = 1.

EOF