Code structure follow up #4 #439
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Rust Benchmark
| Benchmark suite | Current: d73f091 | Previous: af0c65a | Ratio |
|---|---|---|---|
rule-match-browserlike/brave-list |
1699804376 ns/iter (± 9731234) |
1657920369 ns/iter (± 13523152) |
1.03 |
rule-match-first-request/brave-list |
1001595 ns/iter (± 7687) |
970929 ns/iter (± 12529) |
1.03 |
blocker_new/brave-list |
207292155 ns/iter (± 4960395) |
227719857 ns/iter (± 8671111) |
0.91 |
memory-usage/brave-list-initial |
41408849 ns/iter (± 3) |
41408849 ns/iter (± 3) |
1 |
memory-usage/brave-list-after-1000-requests |
44004875 ns/iter (± 3) |
44004875 ns/iter (± 3) |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
| @@ -28,7 +28,7 @@ url = "2.5" | |||
| percent-encoding = "2.1" | |||
| once_cell = "1.8" | |||
| regex = "1.5" | |||
| bitflags = "1.3" | |||
| bitflags = { version = "2.9.0", features = ["serde"] } | |||
There was a problem hiding this comment.
@antonok-edm I’ve bumped the bitflags version because flatbuffers depends on it, and I want to avoid depending on two different versions. However, this change breaks backward compatibility. There is a crate called bitflags-serde-legacy that provides compatibility for older versions. Should I revert this change and stick with 1.3? What do you think?
There was a problem hiding this comment.
IMHO, it makes sense to use the recent 2.9.0 because we're going to break backward compatibility anyway.
There was a problem hiding this comment.
There is a crate called bitflags-serde-legacy that provides compatibility for older versions. Should I revert this change and stick with
1.3? What do you think?
I recall looking into this previously - that crate wasn't even working correctly in my testing, but I wasn't able to find the reason for it.
IMHO, it makes sense to use the recent
2.9.0because we're going to break backward compatibility anyway.
Technically there's backward compatibility w/ the API, and then there's backward compatibility with the serialized format. So far we've been careful to keep the serialized format at least continuously usable across minor adblock-rust versions.
We no longer strictly require either for the sake of Brave Browser. I'm not sure how other downstream dependents of adblock-rust treat the serialized formats but I'm not convinced it's worth it to maintain that compatibility going forwards. It only makes sense in a scenario where the underlying lists are not being distributed; other projects likely also have access to the list text.
There was a problem hiding this comment.
btw, if we're doing this, we may as well also upgrade seahash which was held back for a similar reason (doesn't have to be in the same PR)
There was a problem hiding this comment.
Ok, then I'll update bitflags and seahash crates and fix the corresponding tests.
| table NetworkFilter { | ||
| mask: uint32; // NetworkFilterMask (network.rs) | ||
|
|
||
| // These arrays contain sorted(asc) indecies in the |unique_domains_hashes| |
There was a problem hiding this comment.
indecies => indices
itself => themselves
Maybe something like:
These arrays contain the sorted (ascending) indices of elements in the |unique_domains_hashes| array, rather than the hashes themselves.
| self._tab | ||
| .get::<u32>(NetworkFilter::VT_MASK, Some(0)) | ||
| .unwrap() | ||
| } |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected 'unsafe' usage, please audit for secure usage
Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage
Cc @thypon @kdenhartog
| NetworkFilter::VT_OPT_DOMAINS, | ||
| None, | ||
| ) | ||
| } |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected 'unsafe' usage, please audit for secure usage
Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage
Cc @thypon @kdenhartog
| NetworkFilter::VT_OPT_NOT_DOMAINS, | ||
| None, | ||
| ) | ||
| } |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected 'unsafe' usage, please audit for secure usage
Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage
Cc @thypon @kdenhartog
| self._tab.get::<flatbuffers::ForwardsUOffset< | ||
| flatbuffers::Vector<'a, flatbuffers::ForwardsUOffset<&'a str>>, | ||
| >>(NetworkFilter::VT_PATTERNS, None) | ||
| } |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected 'unsafe' usage, please audit for secure usage
Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage
Cc @thypon @kdenhartog
| NetworkFilter::VT_MODIFIER_OPTION, | ||
| None, | ||
| ) | ||
| } |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected 'unsafe' usage, please audit for secure usage
Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage
Cc @thypon @kdenhartog
| unsafe { | ||
| self._tab | ||
| .get::<flatbuffers::ForwardsUOffset<&str>>(NetworkFilter::VT_HOSTNAME, None) | ||
| } |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected 'unsafe' usage, please audit for secure usage
Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage
Cc @thypon @kdenhartog
| unsafe { | ||
| self._tab | ||
| .get::<flatbuffers::ForwardsUOffset<&str>>(NetworkFilter::VT_TAG, None) | ||
| } |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected 'unsafe' usage, please audit for secure usage
Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage
Cc @thypon @kdenhartog
| flatbuffers::Vector<'a, flatbuffers::ForwardsUOffset<NetworkFilter>>, | ||
| >>(NetworkFilterList::VT_NETWORK_FILTERS, None) | ||
| .unwrap() | ||
| } |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected 'unsafe' usage, please audit for secure usage
Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage
Cc @thypon @kdenhartog
| None, | ||
| ) | ||
| .unwrap() | ||
| } |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected 'unsafe' usage, please audit for secure usage
Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage
Cc @thypon @kdenhartog
src/blocker.rs
Outdated
| use crate::filters::network::{NetworkFilter, NetworkFilterMaskHelper, NetworkMatchable}; | ||
| use crate::optimizer; | ||
| use crate::filters::network::{NetworkFilter, NetworkFilterMaskHelper}; | ||
| pub(crate) use crate::network_filter_list::NetworkFilterList; |
There was a problem hiding this comment.
let's drop the pub(crate) here, and change other imports to refer to it directly from crate::network_filter_list
src/lib.rs
Outdated
| @@ -24,6 +24,7 @@ mod data_format; | |||
| mod engine; | |||
| pub mod filters; | |||
| pub mod lists; | |||
| pub mod network_filter_list; | |||
There was a problem hiding this comment.
we can remove pub here; nothing inside the module is public
data/rs-ABPFilterParserData.dat
Outdated
There was a problem hiding this comment.
Leftover? Do we use it somewhere?
There was a problem hiding this comment.
iOS: I’ve just made the live tests an optional step in the GitHub workflow. At some point, we’ll need to update the CI (or the place where the .dat file for iOS is generated, I’m not sure) to generate the new version. This might also require an update to the browser code.
There was a problem hiding this comment.
@antonok-edm Do you know how and where the latest.dat file for iOS is generated? Is it part of the iOS browser, or is it a downloadable component?
There was a problem hiding this comment.
Leftover? Do we use it somewhere?
Right, we can remove rs-ABPFilterParserData.dat and related tests safely if we are no longer guaranteeing that older serialized versions can be deserialized. We only need to continue to test that the round trip from list text -> serialized -> Engine still works as expected.
Do you know how and where the latest.dat file for iOS is generated?
iOS switched to using plaintext files quite some time ago so that should be safe to remove as well.
There was a problem hiding this comment.
I’ve just made the live tests an optional step in the GitHub workflow.
IIRC some of the live tests use the list text, we should keep those but anything using .dat files can be removed.
boocmp
force-pushed
the
code_structure_5
branch
from
d8f525f to
0c9f488
Compare
Fixed tests. Review issues are addressed.
boocmp
force-pushed
the
code_structure_5
branch
from
0c9f488 to
cf5b754
Compare
|
[puLL-Merge] - brave/adblock-rust@439 DescriptionThis PR makes several significant changes to the adblock-rust codebase:
The main motivation appears to be code organization and modernization while removing company-specific test cases. ChangesChanges
sequenceDiagram
participant Client
participant Blocker
participant NetworkFilterList
participant Request
participant Filter
participant RegexManager
Client->>Blocker: check_network_request(request)
Blocker->>NetworkFilterList: check(request, tags, regex_mgr)
NetworkFilterList->>Request: get_tokens_for_match()
Request-->>NetworkFilterList: tokens
loop For each token
NetworkFilterList->>Filter: matches(request, regex_mgr)
Filter->>RegexManager: check_matches(pattern)
RegexManager-->>Filter: match result
Filter-->>NetworkFilterList: match result
end
NetworkFilterList-->>Blocker: matching filter
Blocker-->>Client: block decision
Security HotspotsThe seahash version upgrade from v3 to v4 introduces a breaking hash algorithm change, which could affect filter matching behavior and introduce edge cases where previously blocked content may pass through. |
|
@antonok-edm Could you please clarify what happens on iOS if we merge this version to b-c? I suppose the browser will fail to decode the store .dat files and compile the engine from plaintext lists? |
NetworkFilterListhas been moved to network_filter_list.rs.bitflagsdependency version has been bumped to 2.9.0.flatbuffersdependency has been added.