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.

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

chore(releasing): Slightly fiddle with release build options, container paths #6614

Merged
merged 4 commits into from
Mar 15, 2021
Merged

chore(releasing): Slightly fiddle with release build options, container paths #6614

merged 4 commits into from
Mar 15, 2021

Conversation

blt
Copy link
Contributor

@blt blt commented Mar 3, 2021

This commit documents the release build options in Cargo.toml without
changing them. It also sets the container paths for builds to absolute paths,
resolving a build issue on my strict podman install.

Signed-off-by: Brian L. Troutwine brian@troutwine.us

@blt blt requested review from a team and vladimir-dd and removed request for a team March 3, 2021 19:06
Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

Thanks @blt !

We actually did an in-depth look at binary size recently: #6202 . I thought we'd actually decided to leave debug symbols in there via removing RUSTFLAGS="-C link-arg=-s", but it seems this still leave some symbols out?

Also over in that issue, I suggested that we build the release builds with debug symbols but strip them from the release binaries and store them as separate artifacts. That would permit us to use them as needed without bloating the overall binary size for normal deployments. What would you think of that?

Cargo.toml Outdated Show resolved Hide resolved
@jszwedko
Copy link
Member

jszwedko commented Mar 3, 2021

As you note in the description, this seems like a substantial increase in size: 70 MB -> 600 MB if I'm reading it right. I hesitate to start releasing 600 MB versions of vector 😅 We could ask users to run them, as needed, if we do run into an issue that would benefit from a debug build.

@blt
Copy link
Contributor Author

blt commented Mar 3, 2021

As you note in the description, this seems like a substantial increase in size: 70 MB -> 600 MB if I'm reading it right. I hesitate to start releasing 600 MB versions of vector sweat_smile

Yep. It's a big jump. It's all of vector's symbols plus every symbol from the crates that aren't shook out of the tree by LTO. The all features build just has a lot of stuff in it, I guess.

We could ask users to run them, as needed, if we do run into an issue that would benefit from a debug build.

I guess, to be clear, I'm less concerned with user experience -- I mean, other than the 600MB file size which is bad -- than I am with dev work. Having symbols available is a big win here for integration with the perf tool. My two concerns are:

  • I don't want to release a mega-binary like this generates and
  • I do want symbols in or with a release build for performance work.

Seems like we could build with symbols by default then strip when we cut a release or publish a separate symbols file. Either approach suits my ambitions.

@jszwedko
Copy link
Member

jszwedko commented Mar 3, 2021

I guess, to be clear, I'm less concerned with user experience -- I mean, other than the 600MB file size which is bad -- than I am with dev work. Having symbols available is a big win here for integration with the perf tool. My two concerns are:

  • I don't want to release a mega-binary like this generates and
  • I do want symbols in or with a release build for performance work.

Seems like we could build with symbols by default then strip when we cut a release or publish a separate symbols file. Either approach suits my ambitions.

Aha, I see. I hadn't realized the intention here was to improve the performance work during development.

I think we are in general agreement. I'm happy to see this go in if we modify the release workflow to strip out the symbols after the fact (preferably storing them as another artifact in S3).

Alternatively, if this is something you only need locally, setting it in ~/.cargo/config.toml could be an option.

One last alternative would be a separate, custom, profile: performance that matches release with additional toggles useful when doing profiling.

@blt
Copy link
Contributor Author

blt commented Mar 3, 2021

One last alternative would be a separate, custom, profile: performance that matches release with additional toggles useful when doing profiling.

This sparks joy.

@blt
Copy link
Contributor Author

blt commented Mar 3, 2021

@jszwedko ah, hmm. Seems like custom profiles are still a work in progress. I'll see about pursuing:

I think we are in general agreement. I'm happy to see this go in if we modify the release workflow to strip out the symbols after the fact (preferably storing them as another artifact in S3).

@jszwedko
Copy link
Member

jszwedko commented Mar 3, 2021

@jszwedko ah, hmm. Seems like custom profiles are still a work in progress.

Oh 😢

I'll see about pursuing:

I think we are in general agreement. I'm happy to see this go in if we modify the release workflow to strip out the symbols after the fact (preferably storing them as another artifact in S3).

👍

https://github.com/timberio/vector/blob/master/.github/workflows/nightly.yml

and

https://github.com/timberio/vector/blob/master/.github/workflows/release.yml

Are probably good starting places.

@binarylogic
Copy link
Contributor

@jszwedko is correct. I just wanted to chime in and confirm that we should not be shipping a 600MB binary to users for releases.

I think we are in general agreement. I'm happy to see this go in if we modify the release workflow to strip out the symbols after the fact (preferably storing them as another artifact in S3).

Agree 👍 .

@blt
Copy link
Contributor Author

blt commented Mar 3, 2021

Hah, yep. Me either. Honestly I'm surprised at how much bigger vector gets when symbols are left in.

@blt
Copy link
Contributor Author

blt commented Mar 3, 2021

Hmm, so I'm going to back off this some. I do have modifications to the Makefile that will use objcopy --only-keep-debug etc to make a split-debuginfo but the work there will end up being very platform specific. I'd rather wait until rust-lang/rust#34651 wraps to reconsider. By not pursuing this we make any nightly that runs in test-harness do so without symbols, which was my initial concern. However, after a walk around the block it occurs to me that the desire to run arbitrary commits through test-harness means we're going to need to build binaries in some manner there anyhow, so, eh, why not punt into the future?

I did make some improvements here that my stricter install of podman complained about, so I'll re-orient the PR in that direction.

@blt
Copy link
Contributor Author

blt commented Mar 4, 2021

Hmm, the tests seem to be consistently failing on OS X https://github.com/timberio/vector/pull/6614/checks?check_run_id=2033017331#step:8:1482. Looks to be related to rate limiting, @jszwedko. FWIW I cannot reproduce locally on Linux and am a little at a loss as to the failure.

@jszwedko
Copy link
Member

jszwedko commented Mar 4, 2021

Hmm, the tests seem to be consistently failing on OS X https://github.com/timberio/vector/pull/6614/checks?check_run_id=2033017331#step:8:1482. Looks to be related to rate limiting, @jszwedko. FWIW I cannot reproduce locally on Linux and am a little at a loss as to the failure.

@blt indeed it does! I will take a look at that.

@jszwedko
Copy link
Member

jszwedko commented Mar 4, 2021

I believe that test should be fixed by #6632

Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

👍 to the dockerfile changes

@blt
Copy link
Contributor Author

blt commented Mar 4, 2021

I believe that test should be fixed by #6632

Cool! I'll rebase once that's in master.

blt added 4 commits March 13, 2021 11:07
This commit adjust `Cargo.toml` to include debug symbols in the release
build. This will increase the final size of our binary but also is required to
make sensible traces of vector internals from external tools. For deployments
that require smaller binaries we can offer a stripped variant, or instructions
on how to achieve this.

Binary size before:

```
 File  .text     Size Crate
11.1%  21.2%   8.4MiB std
 5.2%  10.0%   4.0MiB vector
 4.8%   9.1%   3.6MiB erased_serde
 2.7%   5.1%   2.0MiB [Unknown]
 2.6%   5.0%   2.0MiB h2
 2.3%   4.5%   1.8MiB hyper
 2.3%   4.3%   1.7MiB futures_util
 1.9%   3.6%   1.4MiB tokio
 1.5%   2.9%   1.2MiB serde
 1.2%   2.4% 959.5KiB openssl_sys
 1.0%   2.0% 793.7KiB rustls
 0.7%   1.3% 549.2KiB async_graphql
 0.6%   1.2% 493.1KiB zstd_sys
 0.6%   1.2% 476.5KiB rdkafka_sys
 0.6%   1.2% 469.8KiB hashbrown
 0.6%   1.1% 431.0KiB vrl_stdlib
 0.5%   1.0% 424.5KiB serde_json
 0.5%   1.0% 405.7KiB mongodb
 0.5%   0.9% 370.7KiB vrl_parser
 0.4%   0.8% 310.8KiB vrl_compiler
 9.8%  18.7%   7.4MiB And 242 more crates. Use -n N to show more.
52.1% 100.0%  39.7MiB .text section size, the file size is 76.2MiB
```

Binary size after:

```
File  .text     Size Crate
1.4%  21.4%   8.6MiB std
0.7%   9.9%   4.0MiB vector
0.6%   9.5%   3.8MiB erased_serde
0.3%   5.1%   2.0MiB [Unknown]
0.3%   5.0%   2.0MiB h2
0.3%   4.5%   1.8MiB hyper
0.3%   4.3%   1.7MiB futures_util
0.2%   3.5%   1.4MiB tokio
0.2%   2.9%   1.2MiB serde
0.2%   2.4% 981.2KiB openssl_sys
0.1%   1.9% 795.9KiB rustls
0.1%   1.3% 549.3KiB async_graphql
0.1%   1.2% 505.5KiB zstd_sys
0.1%   1.2% 483.4KiB hashbrown
0.1%   1.2% 476.5KiB rdkafka_sys
0.1%   1.1% 435.3KiB serde_json
0.1%   1.0% 431.0KiB vrl_stdlib
0.1%   1.0% 405.7KiB mongodb
0.1%   0.9% 370.8KiB vrl_parser
0.1%   0.8% 312.0KiB vrl_compiler
1.2%  18.6%   7.5MiB And 242 more crates. Use -n N to show more.
6.6% 100.0%  40.1MiB .text section size, the file size is 606.5MiB
```

For a full build with all features enabled it's a _substantial_
difference in size.

Signed-off-by: Brian L. Troutwine <brian@troutwine.us>
Signed-off-by: Brian L. Troutwine <brian@troutwine.us>
Signed-off-by: Brian L. Troutwine <brian@troutwine.us>
Signed-off-by: Brian L. Troutwine <brian@troutwine.us>
@blt blt changed the title enhancement(performance): Include debug symbols in release build, document options chore(releasing): Slightly fiddle with release build options, container paths Mar 13, 2021
@blt blt merged commit 00b0422 into vectordotdev:master Mar 15, 2021
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.

None yet

3 participants