Flatbuffers impl #446

boocmp · 2025-03-28T08:24:44Z

The NetworkFilterList implementation is now based on FlatBuffers.
The serialization format has been updated to support this change.
Serialization benchmarks have been moved to a separate file.
Support for adding filters after NetworkFilterList creation has been removed. Since rebuilding the entire FlatBuffer is costly, this functionality may be reintroduced in the future if there is a clear need for it.

github-actions

Rust Benchmark

Benchmark suite	Current: `79b0b83`	Previous: `67b7b70`	Ratio
`rule-match-browserlike/brave-list`	`2191632901` ns/iter (`± 16546585`)	`1803081237` ns/iter (`± 16195247`)	`1.22`
`rule-match-first-request/brave-list`	`1049815` ns/iter (`± 25195`)	`980016` ns/iter (`± 5452`)	`1.07`
`blocker_new/brave-list`	`160335860` ns/iter (`± 2521451`)	`203961371` ns/iter (`± 2004867`)	`0.79`
`memory-usage/brave-list-initial`	`21536659` ns/iter (`± 3`)	`41762172` ns/iter (`± 3`)	`0.52`
`memory-usage/brave-list-after-1000-requests`	`24141128` ns/iter (`± 3`)	`44355700` ns/iter (`± 3`)	`0.54`

This comment was automatically generated by workflow using github-action-benchmark.

github-actions · 2025-04-09T00:58:13Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{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

It turns out that from_raw_parts results in issues if bytes.as_ptr() isn't aligned to 2 bytes We need to assert this.

assert added

github-actions · 2025-04-09T00:58:13Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{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

github-actions · 2025-04-09T00:58:13Z

src/flat_network_filter_list.rs

+        }
+
+        let filters_list =
+            unsafe { fb::root_as_network_filter_list_unchecked(&self.flatbuffer_memory) };


_{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

@boocmp use root_as_network_filter_list() + .expect() to remove unsafe

it runs a verification process, it'll degrade performance.

Until we build the flatbuffer just before using root_as_network_filter_list_unchecked should be fine in terms of security.

added this check on deserialization

github-actions · 2025-04-09T00:58:13Z

src/flatbuffers/fb_network_filter_generated.rs

+            unsafe {
+                self._tab
+                    .get::<flatbuffers::ForwardsUOffset<&str>>(NetworkFilter::VT_RAW_LINE, None)
+            }


_{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

atuchin-m

LGTM with a few nits

src/filters/fb_network.rs

.github/workflows/perf-ci.yml

src/filters/fb_network.rs

github-actions · 2025-04-10T11:28:44Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{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

github-actions · 2025-04-10T11:28:44Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{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

antonok-edm · 2025-04-17T18:30:17Z

src/blocker.rs

+        let mut disabled_directives: HashSet<String> = HashSet::new();
+        let mut enabled_directives: HashSet<String> = HashSet::new();


why not &str anymore?

src/data_format/mod.rs

src/engine_serializer.rs

src/filters/fb_network.rs

github-actions · 2025-04-28T07:11:37Z

src/network_filter_list.rs

        if self.filter_map.is_empty() {
            return None;
        }

+        let filters_list =
+            unsafe { fb::root_as_network_filter_list_unchecked(&self.flatbuffer_memory) };


_{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

github-actions · 2025-04-28T07:11:37Z

src/network_filter_list.rs


        if self.filter_map.is_empty() {
            return filters;
        }

+        let filters_list =
+            unsafe { fb::root_as_network_filter_list_unchecked(&self.flatbuffer_memory) };


_{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

github-actions · 2025-04-28T08:02:49Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{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

github-actions · 2025-04-28T08:02:49Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{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

github-actions · 2025-04-29T04:33:41Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{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

antonok-edm · 2025-05-13T16:29:02Z

src/engine_serializer.rs

+pub trait EngineSerializer {
+    fn serialize(&self) -> Result<Vec<u8>, SerializeError>;
+    fn deserialize(&mut self, serialized: &[u8]) -> Result<(), DeserializeError>;
+}
+
+impl EngineSerializer for Engine {


could we leave these directly on impl Engine? I see no reason to move it into a separate trait; just makes the API slightly more confusing for users

antonok-edm · 2025-05-13T16:45:10Z

src/filters/fb_network.rs

+        index: u32,
+        owner: &'a NetworkFilterList,
+    ) -> Self {
+        let list_address: *const NetworkFilterList = owner as *const NetworkFilterList;


pointer casting often indicates some problematic design patterns in Rust. I think we can remove it in this instance?

this owner field can increase memory usage since it's an additional address-sized field on each filter, even though they'd all have the same value for all filters in a list

I only see it used in NetworkMatchable to make it conform to the existing API. I'm not opposed to that API changing if needed.

FlatNetworkFilter is a wrapper over the fb::NetworkFilter. We only create the instance of it when found the the fb::NetworkFilter in the NetworkFilterList so it does't increase the memory usage because it temporary object on the stack. For now, regex manager is designed to use unique key to find corresponding regex, previously it was an address of the NetworkFilter, but now we don't have unique address for every filter because they stored in flatbuffer. To create unique key we use the list address combined with the filter's index. I agree it looks ugly but I don't know how to do it better.

Let's understand why we need this unique key.
If it's only for regex manager, let's:

try to move key calculation here

add TODO to make keys unique only in scope of regex manager. I believe once that in one of the next steps we move from multiple flatbuffers to a just one that ables to contain multiple lists. That way we get 1:1 proportion (regex manager: flatbuffer).

Also, this could be done in follow up: this PR is already waiting for a long time.

antonok-edm · 2025-05-13T16:46:55Z

src/filters/fb_network.rs

+                patterns: patterns,
+                modifier_option: modifier_option,
+                opt_domains: opt_domains,
+                opt_not_domains: opt_not_domains,
+                hostname: hostname,
+                tag: tag,
+                raw_line: raw_line,


these can all use shorthand field initialization

antonok-edm · 2025-05-13T16:51:12Z

src/network_filter_list.rs

+#[derive(Serialize, Deserialize)]
+pub struct NetworkFilterList {
+    pub(crate) flatbuffer_memory: Vec<u8>,
+    pub(crate) filter_map: HashMap<Hash, Vec<u32>>,
+    pub(crate) unique_domains_hashes_map: HashMap<Hash, u16>,
+}
+
+impl Default for NetworkFilterList {
+    fn default() -> Self {
+        Self {
+            flatbuffer_memory: Default::default(),
+            filter_map: Default::default(),
+            unique_domains_hashes_map: Default::default(),
+        }
+    }


#[derive(Serialize, Deserialize, Default)] should automatically produce this Default implementation already

For some reason it doesn't, I think because of the new version of serde

antonok-edm · 2025-05-13T16:52:46Z

src/network_filter_list.rs

+}
+
+#[derive(Serialize, Deserialize)]
+pub struct NetworkFilterList {


can we keep the original pub(crate) visibility?

antonok-edm · 2025-05-13T16:56:52Z

tests/unit/blocker.rs

-        assert_eq!(added.err().unwrap(), BlockerError::BadFilterAddUnsupported);
-    }
-
-    #[test]
-    #[ignore]
-    fn filter_add_twice_handling_error() {
-        {
-            // Not allow filter to be added twice hwn the engine is not optimised
-            let blocker_options: BlockerOptions = BlockerOptions {
-                enable_optimizations: false,
-            };
-
-            let mut blocker = Blocker::new(Vec::new(), &blocker_options);
-
-            let filter = NetworkFilter::parse("adv", true, Default::default()).unwrap();
-            blocker.add_filter(filter.clone()).unwrap();
-            assert!(
-                blocker.filter_exists(&filter),
-                "Expected filter to be inserted"
-            );
-            let added = blocker.add_filter(filter);
-            assert!(added.is_err(), "Expected repeated insertion to fail");
-            assert_eq!(
-                added.err().unwrap(),
-                BlockerError::FilterExists,


BadFilterAddUnsupported and FilterExists error variants can be removed now

github-actions · 2025-05-16T07:39:31Z

[puLL-Merge] - brave/adblock-rust@446

Description

This PR changes the internal representation of network filters in the adblock-rust library, switching to using flatbuffers for serialization instead of the previous approach. The main goal appears to be improving performance and storage efficiency. The PR also refactors the serialization benchmarks into a separate file and makes flatbuffers a required dependency rather than an optional one.

Changes

Changes

Cargo.toml
- Makes flatbuffers a required dependency (no longer optional)
- Removes the optional flatbuffers-storage feature
- Adds a new benchmark for serialization
Benchmarks
- Moves serialization/deserialization benchmarks from bench_matching.rs to a new file bench_serialization.rs
- Updates serialization benchmark to use the new serialize() method instead of serialize_raw()
FlatBuffers Implementation
- Adds a new module fb_network.rs that implements flatbuffers-based network filter storage
- Updates the flatbuffers schema to include a raw_line field
Engine API
- Renames serialize_raw() to serialize() and deserialize() remains the same
- Makes blocker and cosmetic_cache fields public(crate)
- Adjusts internal implementation to use the new flatbuffers approach
Blocker Implementation
- Removes methods optimize(), add_filter(), and filter_exists() from Blocker
- Changes internal filter storage to use indices into the flatbuffer instead of Arc pointers
Network Filter Matching
- Refactors check_options, splitting it into separate functions for domain inclusion/exclusion
- Adds versions of these functions that work with the new flatbuffers storage
Examples and Tests
- Updates examples to use the new API
- Updates tests to work with the new implementation

sequenceDiagram
    participant C as Client
    participant E as Engine
    participant B as Blocker
    participant FB as FlatNetworkFiltersListBuilder
    participant NFList as NetworkFilterList

    C->>E: create Engine
    E->>B: create Blocker
    B->>FB: new FlatNetworkFiltersListBuilder
    loop For each network filter
        B->>FB: add filter
        FB-->>B: filter index
    end
    FB->>B: finish() (creates flatbuffer)
    B-->>E: return Blocker with flatbuffer storage

    C->>E: serialize()
    E->>E: create SerializeFormat with blocker & cosmetic_cache
    E-->>C: return binary data

    C->>E: deserialize(binary)
    E->>E: parse and rebuild internal state from binary
    E-->>C: return success/error
    
    C->>E: check_network_request(request)
    E->>B: check(request, resources)
    B->>NFList: match_rule(request, active_tags, regex_manager)
    NFList->>FB: get network_filter by index
    FB-->>NFList: return FlatNetworkFilter
    NFList->>NFList: check if filter matches request
    NFList-->>B: return CheckResult
    B-->>E: return BlockerResult
    E-->>C: return result

Possible Issues

Some methods have been removed from the Blocker class that may have been used by external code, such as optimize(), add_filter(), and filter_exists().
The PR introduces a significant change in the internal storage format, which might result in compatibility issues with existing serialized engines.
The error types and debug information have changed, which could affect error handling in client code.

Security Hotspots

No significant security concerns were identified. The PR mainly changes internal storage mechanisms rather than security-sensitive components.

src/data_format/v0.rs

Added raw_line field to flatbuffers for testing.

github-actions bot reviewed Mar 28, 2025

View reviewed changes

boocmp force-pushed the flatbuffers_impl branch from 91c5ec7 to cacfcc6 Compare April 9, 2025 00:55

github-actions bot reviewed Apr 9, 2025

View reviewed changes

github-actions bot added the needs-security-review label Apr 9, 2025

github-actions bot assigned kdenhartog and thypon Apr 9, 2025

boocmp force-pushed the flatbuffers_impl branch from cacfcc6 to a3df20d Compare April 9, 2025 01:05

boocmp marked this pull request as ready for review April 9, 2025 01:38

boocmp requested review from atuchin-m and antonok-edm April 9, 2025 01:38

atuchin-m approved these changes Apr 9, 2025

View reviewed changes

boocmp force-pushed the flatbuffers_impl branch from 6a55c00 to a3df20d Compare April 9, 2025 11:30

github-actions bot added the puLL-Merge label Apr 9, 2025

github-actions bot reviewed Apr 10, 2025

View reviewed changes

antonok-edm reviewed Apr 18, 2025

View reviewed changes

github-actions bot reviewed Apr 28, 2025

View reviewed changes

boocmp force-pushed the flatbuffers_impl branch 4 times, most recently from 0238db7 to a40fa5e Compare April 28, 2025 07:50

github-actions bot reviewed Apr 28, 2025

View reviewed changes

github-actions bot reviewed Apr 29, 2025

View reviewed changes

antonok-edm reviewed May 13, 2025

View reviewed changes

boocmp requested review from antonok-edm and atuchin-m May 16, 2025 08:00

atuchin-m reviewed May 16, 2025

View reviewed changes

src/data_format/v0.rs Outdated Show resolved Hide resolved

atuchin-m approved these changes May 16, 2025

View reviewed changes

boocmp added 2 commits May 16, 2025 18:38

Added NetworkFilterListTrait to futher provide the flatbuffer impl.

76bedcf

Added FlatNetworkFilterList implementation.

0860f67

Added raw_line field to flatbuffers for testing.

boocmp added 12 commits May 16, 2025 18:38

Blocker type for data_format lib.

7f86186

Added EngineSerializer trait

949f816

Renaming.

05db953

Review.

9fb57fc

NetworkFilterList impl replaced with flatbuffers.

b87f0f6

Review.

dd775dc

Added checks for flatbuffers structures.

da6a0ee

Removed BlockerError.

bf03647

pub(crate) for NetworkFilterList.

730695d

Shorthand init.

56202a7

Removed EngineSerializer trait.

3c816ca

Deserialise returns Result<...> now.

79b0b83

boocmp force-pushed the flatbuffers_impl branch from a541821 to 79b0b83 Compare May 16, 2025 11:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flatbuffers impl #446

Flatbuffers impl #446

boocmp commented Mar 28, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot Apr 9, 2025

atuchin-m Apr 9, 2025

boocmp Apr 29, 2025

github-actions bot Apr 9, 2025

github-actions bot Apr 9, 2025

atuchin-m Apr 9, 2025

boocmp Apr 28, 2025

atuchin-m Apr 28, 2025

boocmp Apr 29, 2025

github-actions bot Apr 9, 2025

atuchin-m left a comment

github-actions bot Apr 10, 2025

github-actions bot Apr 10, 2025

antonok-edm Apr 17, 2025

github-actions bot Apr 28, 2025

github-actions bot Apr 28, 2025

github-actions bot Apr 28, 2025

github-actions bot Apr 28, 2025

github-actions bot Apr 29, 2025

antonok-edm May 13, 2025

antonok-edm May 13, 2025

boocmp May 16, 2025

atuchin-m May 16, 2025

atuchin-m May 16, 2025

antonok-edm May 13, 2025

antonok-edm May 13, 2025

boocmp May 16, 2025

antonok-edm May 13, 2025

antonok-edm May 13, 2025

github-actions bot commented May 16, 2025

Changes

		let mut disabled_directives: HashSet<String> = HashSet::new();
		let mut enabled_directives: HashSet<String> = HashSet::new();

Flatbuffers impl #446

Are you sure you want to change the base?

Flatbuffers impl #446

Conversation

boocmp commented Mar 28, 2025 • edited Loading

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

Rust Benchmark

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

atuchin-m left a comment

Choose a reason for hiding this comment

github-actions bot Apr 10, 2025

Choose a reason for hiding this comment

github-actions bot Apr 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Apr 28, 2025

Choose a reason for hiding this comment

github-actions bot Apr 28, 2025

Choose a reason for hiding this comment

github-actions bot Apr 28, 2025

Choose a reason for hiding this comment

github-actions bot Apr 28, 2025

Choose a reason for hiding this comment

github-actions bot Apr 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 16, 2025

Description

Changes

Possible Issues

Security Hotspots

boocmp commented Mar 28, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading