-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Experiment ac rust/v6 #9904
Experiment ac rust/v6 #9904
Conversation
Avoid redundant pointer checks; instead check once.
So in conclusion, it seems that "AC-RS" in this PR is better than our "AC-BS", but not nearly as good as our regular "AC" and "AC-KS". Hyperscan is even better than those. I think I'd like to merge this still, perhaps replacing "ac-bs" so we can revisit this as Rust and/or the aho-corasick crate improve. |
Count uniq matches only, like the Hyperscan implementation. Ticket: OISF#3847.
efff325
to
4e722ba
Compare
OTOH maybe just remove ac-bs and keep this work as a branch to revisit every once in a while. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9904 +/- ##
==========================================
- Coverage 82.45% 82.29% -0.16%
==========================================
Files 972 973 +1
Lines 273057 273878 +821
==========================================
+ Hits 225156 225400 +244
- Misses 47901 48478 +577
Flags with carried forward coverage won't be shown. Click here to find out more. |
Yeah, was going to ask, whats the purpose of keeping ac-bs? |
} | ||
|
||
#[no_mangle] | ||
pub extern "C" fn rs_mpm_acrs_new_builder() -> *mut std::os::raw::c_void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the unsafe notes from the previous PR... We've been doing C style naming for functions that form the C API recently.. So SCMpmAcRsNewBuilder
or whatever.
I guess this kind of breaks the way we've been looking at the impact of Rust functions tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions would not need to be part of a public C API. C API should only expose it through the generic MPM support. So would that mean we drop the SC? MpmAcRsNewBuilder
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If its no_mangle extern "C" its part of the public API, as far as linking goes which is why we want to name space under SC
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm can we have a way to make things not public from a libsuri perspective? This stuff is public not because I want it to be, but because the C-Rust FFI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name, like SCPrivate...
or _SC
to make it clear its for private use only. Additionally removing or ifdef
out of the public headers, either would require some creative use or post-processing of the cbindgen output. The namespacing is to avoid a collision, and the name scheme or ifdef is to avoid unintentional linking. Nothing we can do about intentional linking.
ERROR: ERROR: QA failed on TREX_GENERIC_collect. Pipeline 16748 |
Will do a few more tests to see if there is hw where |
|
closing in favor of #9911 |
Some results:
AMD Ryzen 7 PRO 4750U
ET Open
So HS > AC-KS > AC -> AC-RS > AC-BS.
ET pro
So HS > AC-KS > AC > AC-RS > AC-BS.
Intel Xeon W-2245
ET Open
So HS > AC > AC-KS > AC-RS > AC-BS.
ET Pro
So HS > AC > AC-KS > AC-RS > AC-BS.
Rock 5b (arm64)
ET Open
AC > AC-KS > AC-RS > AC-BS.
Results are in a different unit than ticks in our arm64 code.