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

hyperscan: skip compilation on unused libs #27534

Merged
merged 2 commits into from
May 22, 2023
Merged

Conversation

zhxie
Copy link
Contributor

@zhxie zhxie commented May 22, 2023

Signed-off-by: Xie Zhihao [email protected]

Commit Message: hyperscan: skip compilation on unused libs
Additional Description:
Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #27534 was opened by zhxie.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label May 22, 2023
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @phlax

🐱

Caused by: #27534 was opened by zhxie.

see: more, trace.

Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

thanks for taking a look at this @zhxie - really appreciated

@zhxie
Copy link
Contributor Author

zhxie commented May 22, 2023

There is not too much unused libs to remove.

In fact, Hyperscan does not have many targets to compile, but it compiles a separate copy of the code for each platform (SSE4, AVX, AVX2...) in order to optimize performance on different platforms.

Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

thanks for following up on this @zhxie, lgtm

locally my comparison (test of 1) timings:

before:

INFO: Elapsed time: 399.542s, Critical Path: 399.16s

after

INFO: Elapsed time: 347.085s, Critical Path: 338.38s

as this definitely does build on all cores - i think we should try and figure out how to get the ci machines to build it locally rather than compiling with RBE

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label May 22, 2023
@zhxie zhxie marked this pull request as ready for review May 22, 2023 07:12
@phlax phlax enabled auto-merge (squash) May 22, 2023 07:13
@phlax phlax merged commit 6de1595 into envoyproxy:main May 22, 2023
74 of 75 checks passed
@zhxie zhxie deleted the hscan-minify branch May 23, 2023 01:33
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
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

2 participants