envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
24.89k stars 4.79k forks source link

Add Vectorscan (same ABI as Hyperscan) for Arm 64 #29276

Closed sambercovici closed 10 months ago

sambercovici commented 1 year ago

Title: Add Hyperscan/Vectorscan to non x86 processors like Arm 64

Description: https://github.com/envoyproxy/envoy/issues/19376 added hyperscan as a matcher and an optional regular expression engine. Hyperscan supports x86 processors and is included only in contib build for intel.

Vectorscan - https://github.com/VectorCamp/vectorscan is a fork of Hyperscan supporting the same ABI for Arm64 and PowerPC 64. Specificaly:

Either switch to Vectorscan for all supported processors (including Intel), or use Vectorscan for supported non intel processors.

Describe the desired behavior, what scenario it enables and how it would be used. The Hyperscan functionality will be available for all Envoy supported CPU architectures.

[optional Relevant Links:] https://github.com/VectorCamp/vectorscan

Any extra documentation required to understand the issue.

phlax commented 1 year ago

cc @zhxie @soulxu

zhxie commented 1 year ago

Hyperscan supports only intel processors and is included only in contib build for intel.

It is incorrect. Hyperscan supports all x86 processors with SSSE3.

sambercovici commented 1 year ago

Feedback would be appreciated.

sambercovici commented 1 year ago

@phlax is this of interest? Anyone else from the core team may be interested? If this feature is needed, we will implement it.

phlax commented 1 year ago

codeowners cced above are the best people to ask i think

sambercovici commented 1 year ago

@phlax, as far as I see, @zhxie @soulxu work for intel. Should they care about extending the support for other kinds of CPUs?

zhxie commented 1 year ago

Compared to modifying existing components, my suggestion is to add a new extension.

Furthermore, I have concerns about the supply chain security of this component. It appears that this project lacks CI assurance, proper unit testing and Fuzzing testing.

soulxu commented 1 year ago

@phlax, as far as I see, @zhxie @soulxu work for intel. Should they care about extending the support for other kinds of CPUs?

As intel, we aren't interesting other CPUs. For hyperscan, we got the team support, if we move to vectorscan, we won't have any support when something wrong. So it is really hard for us to migration to vectorscan. If anyone has requirement on that and got enough people support that, I think people can just free to add an extension for it.

sambercovici commented 1 year ago

Hi @zhxie, This is proposal is to enable the existing extension for additional CPUs over x64. This means that for x64 the dependency is kept on Hyperscan. For aarch64 and other CPUs the dependency will be on Vectorscan. The envoy extension itself will not change.

@soulxu , I am not asking you/intel to migrate to Vectorscan. Please keep the great job you do on Hyperscan and as suggested the x64 builds will use Hyperscan.

markos commented 1 year ago

@zhxie Hi, I was requested to comment here in favour of Vectorscan. I am only going to comment on the CI and unit testing. So, as Vectorscan is a direct API/ABI Hyperscan replacement, is based on the same code, it runs the same unit tests (both unit-internal and unit-hyperscan are run). There is an internal CI, jenkins based. It is internal only because we didn't integrate it with Github, but we are currently moving to a different CI, something easier to program than Jenkins and easier to integrate to Github. Even so, our current Jenkins tests x86 (SSE, AVX2, AVX512, FAT), Aarch64 (Neon), PPC64LE (VSX). Each architecture is tested for both debug/release, on multiple gcc and clang versions. Each PR is tested against that if it doesn't pass it doesn't get merged. Unfortunately, we haven't integrated that with github so it doesn't show. So, to answer that, the plan is extend our CI to more configurations, more compilers, a few more architectures, add ASAN testing, and of course make it visible to Github. A new release 5.4.10 will be soon (today?) out the door, and a new Debian package will uploaded immediately. The decision to support it is yours of course, I'm just mentioning the current situation.

sambercovici commented 1 year ago

@phlax , @soulxu , @zhxie , any additional comments? If not, may we work on a PR to add VectorScan as the implementation for aarch64 processors?

soulxu commented 1 year ago

@soulxu , I am not asking you/intel to migrate to Vectorscan. Please keep the great job you do on Hyperscan and as suggested the x64 builds will use Hyperscan.

Ok, I see your proposal now. I guess it is same answer, we prefer to hyperscan for x86 build. you are free to do anything for other platform.

github-actions[bot] commented 11 months ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

sambercovici commented 11 months ago

who can tag as "no stalebot"?

phlax commented 11 months ago

done

sambercovici commented 11 months ago

@soulxu , @zhxie Following https://github.com/envoyproxy/envoy/pull/29881#issuecomment-1786455068 What are is the plan for Hyperscan 5.4 LTS since 5.5 and forward will be under "Intel Proprietary License"?