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

Jump to bottom

compressor: Allow to optionally use zlib-ng #12408

Merged
merged 50 commits into from Aug 26, 2020
Merged

compressor: Allow to optionally use zlib-ng #12408

merged 50 commits into from Aug 26, 2020

Conversation

dio
Copy link
Member

@dio dio commented Jul 31, 2020 •
edited

Commit Message: This patch enables an option to build the compressor library using zlib-ng through --define zlib=ng Bazel option. This is available for Linux only.

Motivated by @rojkov's performance benchmark result in #8448 (comment)

Signed-off-by: Dhi Aurrahman dio@tetrate.io

Additional Description: For quickly (and always) enabling zlib_ng, add build --define zlib=ng in your user.bazelrc. The
test run https://github.com/envoyproxy/envoy/runs/938892609 confirms it works for the safe zlib-ng.
Risk Level: Low, optional.
Testing: Existing.
Docs Changes: Added.
Release Notes: Added.

@dio
Copy link
Member Author

dio commented Jul 31, 2020

cc. @rgs1

@rgs1
Copy link
Member

rgs1 commented Jul 31, 2020

cc: @fishcakez for bazel-isms

dio added 2 commits August 1, 2020 10:51
@dio
build: Add option to use zlib-ng as compression provider
a6baa68 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Test if zlib=ng works on CI
eb6adb1 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
dio
dio commented Aug 1, 2020
View reviewed changes
.bazelrc Outdated Show resolved Hide resolved
@dio
ZLIB_COMPAT
d73d356 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Copy link
Member Author

dio commented Aug 1, 2020

Hum, seems like checksum is behaving weird using zlib-ng. I must be missing something obvious.

dio added 2 commits August 1, 2020 05:55
@dio
Skip windows for now
d263984 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
WITH_OPTIM off
6dc37c9 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Copy link
Member Author

dio commented Aug 1, 2020 •
edited

@rgs1 if you feel adventurous, please try building Envoy with: build --define zlib=ng-with-optimization.

dio added 4 commits August 1, 2020 07:22
@dio
Update doc
7f55c63 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Strict compat, no optimization, no new strategies
9c8a4ec 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
More comments
4c89236 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Fix format
a92cc2b 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@rgs1
Copy link
Member

rgs1 commented Aug 1, 2020

@rgs1 if you feel adventurous, please try building Envoy with: build --define zlib=ng-with-optimization.

Sweet -- will give it a spin. Thanks D!

dio added 8 commits August 2, 2020 00:36
@dio
Limit to clang and fix asan
e7dae79 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
@bazel_tools//tools/cpp:clang constraint_values
2c1cbdb 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
zlib_ng_with_optimization too
d656fd5 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Fix gcc
6f41044 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
zlib=ng
6b490e6 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Not sure if this is the fix
804dcff 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Only for Linux:x86_64 for now
f172baf 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Checking on ARM64
ce49e45 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Copy link
Member Author

dio commented Aug 2, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dio
Copy link
Member Author

dio commented Aug 2, 2020

Got the tcp_tunnelling failure for arm64, so I think this passes for arm64 as well.

@dio
Copy link
Member Author

dio commented Aug 3, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dio
Remove turning on zlib-ng
49aeb5b 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Remove zlib=ng_with_optimizations
3cfd480 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
lizan
lizan reviewed Aug 25, 2020
View reviewed changes
bazel/envoy_build_system.bzl Outdated Show resolved Hide resolved
@dio
Copy link
Member Author

dio commented Aug 25, 2020

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Cannot retry non-completed check: envoy-presubmit (Linux-x64 compile_time_options), please wait.

🐱

Caused by: a #12408 (comment) was created by @dio.

see: more, trace.

@dio
Copy link
Member Author

dio commented Aug 25, 2020

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #12408 (comment) was created by @dio.

see: more, trace.

mattklein123
mattklein123 previously approved these changes Aug 25, 2020
View reviewed changes
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.

LGTM. Will defer to @lizan for final bazel review. Thanks!

@mattklein123 mattklein123 assigned lizan and unassigned junr03 and mattklein123 Aug 25, 2020
lizan
lizan requested changes Aug 26, 2020
View reviewed changes
bazel/envoy_build_system.bzl Outdated Show resolved Hide resolved
@dio
Attempt
008aec2 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Merge cache entries
046fd0d 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Remove top level alias
0161406 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Minimize the diff
71cbe99 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Trim
cc9ab53 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
grpc
bf8ece6 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Fix
ff08725 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Fix format
8a364a1 
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Copy link
Member Author

dio commented Aug 26, 2020

/retest

Windows failure seems unrelated.

@repokitteh-read-only
Copy link

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #12408 (comment) was created by @dio.

see: more, trace.

@dio dio requested a review from lizan August 26, 2020 09:43
@dio dio merged commit c3c753c into envoyproxy:master Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moderation moderation moderation left review comments

@rgs1 rgs1 rgs1 left review comments

@lizan lizan lizan approved these changes

@junr03 junr03 junr03 left review comments

@mattklein123 mattklein123 mattklein123 left review comments

Assignees

@lizan lizan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dio @rgs1 @junr03 @mattklein123 @moderation @lizan