Skip to content

Conversation

tonya11en
Copy link
Member

Add an option to build a binary that is optimized for size with -Os. Appending --define optimize_binary_size=enabled to Bazel build will override the optimization level.

Risk Level:
Low

Testing:
Manual testing.

Docs Changes:
Release notes and build docs.

Release Notes:
Done.

Tony Allen added 2 commits May 15, 2019 17:13
Add an option to build a binary that is optimized for size with -Os.

Signed-off-by: Tony Allen <tallen@lyft.com>
Signed-off-by: Tony Allen <tallen@lyft.com>
@lizan lizan self-assigned this May 16, 2019
@@ -75,6 +75,9 @@ def envoy_copts(repository, test = False):
}) + select({
repository + "//bazel:disable_signal_trace": [],
"//conditions:default": ["-DENVOY_HANDLE_SIGNALS"],
}) + select({
repository + "//bazel:optimize_bin_size": ["-Os"],
Copy link
Member

@lizan lizan May 16, 2019

Choose a reason for hiding this comment

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

Did you compare this vs passing --copt=-Os to bazel? envoy_copts doesn't apply to dependencies but CLI copt is. So I think --copt=-Os may produce smaller binary.

Copy link
Member

Choose a reason for hiding this comment

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

+1, we need to compile all dependencies with -Os also. Also, can we make sure we are compiling with symbols also? They can be stripped after.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Nice, awesome to see this in progress.

/wait

@@ -75,6 +75,9 @@ def envoy_copts(repository, test = False):
}) + select({
repository + "//bazel:disable_signal_trace": [],
"//conditions:default": ["-DENVOY_HANDLE_SIGNALS"],
}) + select({
repository + "//bazel:optimize_bin_size": ["-Os"],
Copy link
Member

Choose a reason for hiding this comment

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

+1, we need to compile all dependencies with -Os also. Also, can we make sure we are compiling with symbols also? They can be stripped after.

Tony Allen added 3 commits May 16, 2019 11:17
Signed-off-by: Tony Allen <tallen@lyft.com>
wip
Signed-off-by: Tony Allen <tallen@lyft.com>
Signed-off-by: Tony Allen <tallen@lyft.com>
@tonya11en
Copy link
Member Author

Using --copt="-Os" shaved off another 1MB. Thanks for the tip, I wasn't aware it didn't propagate.

I left the option for compiling just Envoy in case somebody wants to do that. Let me know if you think I should rip it out.

@mattklein123
Copy link
Member

I left the option for compiling just Envoy in case somebody wants to do that. Let me know if you think I should rip it out.

I would just rip it out. Can you also document this new stuff in https://github.com/envoyproxy/envoy/blob/master/bazel/README.md? Thank you!

/wait

@lizan
Copy link
Member

lizan commented May 17, 2019

I left the option for compiling just Envoy in case somebody wants to do that. Let me know if you think I should rip it out.

Yeah, I think you can rip it out and add a config to .bazelrc, and document the config in bazel/README.md.

Signed-off-by: Tony Allen <tallen@lyft.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks few more comments.

/wait

Signed-off-by: Tony Allen <tallen@lyft.com>
Signed-off-by: Tony Allen <tallen@lyft.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks, nice addition and cleanup. A few small comments.

/wait

Signed-off-by: Tony Allen <tallen@lyft.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Nice, thanks. Few more comments.

/wait

Signed-off-by: Tony Allen <tallen@lyft.com>
Signed-off-by: Tony Allen <tallen@lyft.com>
@mattklein123
Copy link
Member

@tonya11en in the future friendly request to not force push. It makes reviews much harder. Thanks!

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@mattklein123 mattklein123 merged commit 917e072 into envoyproxy:master May 22, 2019
mpuncel added a commit to mpuncel/envoy that referenced this pull request May 22, 2019
* master: (65 commits)
  proto: Add PATCH method to RequestMethod enum (envoyproxy#6737)
  exe: drop unused deps on zlib compressor code (envoyproxy#7022)
  coverage: fix some misc coverage (envoyproxy#7033)
  Enable proto schema for router_check_tool (envoyproxy#6992)
  stats: rework stat sink flushing to centralize counter latching (envoyproxy#6996)
  [test] convert lds api test config stubs to v2 (envoyproxy#7021)
  router: scoped rds (2c): implement scoped rds API (envoyproxy#6932)
  build: Add option for size-optimized binary (envoyproxy#6960)
  test: adding an integration test framework for file-based LDS (envoyproxy#6933)
  doc: update obsolete ref to api/XDS_PROTOCOL.md (envoyproxy#7002)
  dispatcher: faster runOnAllThreads (envoyproxy#7011)
  example: add csrf sandbox (envoyproxy#6805)
  fix syntax of gcov exclusion zone. (envoyproxy#7023)
  /runtime_modify: add support for query params in body (envoyproxy#6977)
  stats: Create stats for http codes with the symbol table. (envoyproxy#6733)
  health check: fix more fallout from inline deletion change (envoyproxy#6988)
  Max heap fix (envoyproxy#7016)
  Add support to unregister from lifecycle notifications (envoyproxy#6984)
  build spdy_core_alt_svc_wire_format (envoyproxy#7010)
  ext_authz: Make sure initiateCall only called once (envoyproxy#6949)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
@tonya11en tonya11en deleted the size_opt branch September 25, 2019 08:41
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.

3 participants