Flat containers #498

boocmp · 2025-08-11T12:20:23Z

This is a preparation PR for moving cosmetic filters into flatbuffers.

Replaced FlatFilterMap with FlatMultiMapView - a common container that will also be used in CosmeticFilterCache
Introduced FlatSetView (currently unused), which will be used in CosmeticFilterCache
Moved flatbuffers utilities to a flatbuffers folder

github-actions

Rust Benchmark

Benchmark suite	Current: `82b6f4f`	Previous: `d2e870c`	Ratio
`rule-match-browserlike/brave-list`	`2272443061` ns/iter (`± 8891407`)	`2218369928` ns/iter (`± 12441920`)	`1.02`
`rule-match-first-request/brave-list`	`997270` ns/iter (`± 14535`)	`993380` ns/iter (`± 27500`)	`1.00`
`blocker_new/brave-list`	`151879453` ns/iter (`± 3794682`)	`150901091` ns/iter (`± 1059000`)	`1.01`
`blocker_new/brave-list-deserialize`	`68005495` ns/iter (`± 2998873`)	`63992983` ns/iter (`± 1761826`)	`1.06`
`memory-usage/brave-list-initial`	`16282069` ns/iter (`± 3`)	`16225933` ns/iter (`± 3`)	`1.00`
`memory-usage/brave-list-initial/max`	`64817658` ns/iter (`± 3`)	`64817658` ns/iter (`± 3`)	`1`
`memory-usage/brave-list-initial/alloc-count`	`1514486` ns/iter (`± 3`)	`1514650` ns/iter (`± 3`)	`1.00`
`memory-usage/brave-list-1000-requests`	`2516503` ns/iter (`± 3`)	`2505592` ns/iter (`± 3`)	`1.00`
`memory-usage/brave-list-1000-requests/alloc-count`	`66572` ns/iter (`± 3`)	`66070` ns/iter (`± 3`)	`1.01`

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

src/flatbuffers/containers/flat_multimap.rs

src/flatbuffers/containers/indexable.rs

Cargo.toml

src/flatbuffers/containers/sorted_index.rs

Copilot

Pull Request Overview

This PR refactors flatbuffers utilities to use common container implementations in preparation for moving cosmetic filters to flatbuffers. It replaces the existing FlatFilterMap with a more generic FlatMultiMapView that can be reused across different components.

Replaced FlatFilterMap with FlatMultiMapView - a more generic multimap container
Introduced FlatSetView (currently unused) for future use in CosmeticFilterCache
Moved flatbuffers utilities from filters module to dedicated flatbuffers module

Reviewed Changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/network_filter_list.rs	Updated to use new `FlatMultiMapView` instead of `FlatFilterMap`
src/flatbuffers/	New module containing container implementations and utilities
src/filters/	Removed old `flat_filter_map.rs` and `unsafe_tools.rs` modules
tests/	Added comprehensive tests for new container implementations
Cargo.toml	Added clippy lint allowance for format args

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-08-12T18:28:02Z

src/flatbuffers/containers/flat_multimap.rs

+            Some(FlatMultiMapViewIterator {
+                index,
+                key,
+                keys: self.keys.clone(), // Cloning is 3-4% faster than & in benchmarks


[nitpick] The comment claims cloning is faster than references, but this seems counterintuitive and may be a premature optimization. Consider providing more context about why cloning would be faster or benchmark conditions that led to this conclusion.

Suggested change

keys: self.keys.clone(), // Cloning is 3-4% faster than & in benchmarks

keys: self.keys.clone(), // Clone keys for iterator; see implementation details for performance considerations

Copilot · 2025-08-12T18:28:02Z

src/flatbuffers/containers/utils.rs

+
+impl<'a> PartitionPoint<'a, &str> for Vector<'a, ForwardsUOffset<&str>> {
+    fn lower_bound(&self, key: &&str) -> usize {
+        partition_point_fbvector(self, |x| *x < key)


The parameter type &&str (double reference) is unusual and potentially confusing. Consider using &str instead to match the pattern of other implementations.

Suggested change

partition_point_fbvector(self, |x| *x < key)

fn lower_bound(&self, key: &str) -> usize {

partition_point_fbvector(self, |x| x < key)

Copilot · 2025-08-12T18:28:03Z

src/flatbuffers/containers/utils.rs

+
+impl<'a> PartitionPoint<'a, &str> for Vector<'a, ForwardsUOffset<&str>> {
+    fn lower_bound(&self, key: &&str) -> usize {
+        partition_point_fbvector(self, |x| *x < key)


The comparison *x < key dereferences x which is already a &str, but key is &&str. This should be x < *key to compare &str with &str.

Suggested change

partition_point_fbvector(self, |x| *x < key)

partition_point_fbvector(self, |x| x < *key)

antonok-edm

approved assuming the above are addressed

Cargo.toml

antonok-edm · 2025-08-13T01:42:03Z

tests/live.rs

@@ -217,6 +217,7 @@ fn check_live_from_filterlists() {

 #[cfg(feature = "resource-assembler")]
 #[test]
+#[ignore = "issues/499"]


also fixed by #503, this test should not be ignored

the bad thing that we have 2 branches now.
We fixed this into master, but this PR related only to 0.11.x.
I've manually cherry-picked and pushed #502 and #503 to 0.11.x to address this.

I believe we need to clarify our branching strategy here.

antonok-edm · 2025-08-13T01:44:04Z

src/flatbuffers/containers/flat_multimap.rs

+/// A map-like container that uses flatbuffer references.
+/// Provides O(log n) lookup time using binary search on the sorted index.
+/// I is a key type, Keys is specific container of keys, &[I] for fast indexing (u32, u64)
+/// and flatbuffers::Vector<I> if there is no conversion from Vector (str) to slice.


Since we're accumulating several of these flatbuffer utility structs, I'm wondering if it makes sense to create and publish a new separate crate just for those? I imagine others in the Rust community could find them useful too.

No need to do so here, but something to consider.

That could be an option once we finally stabilize the interface and merge it to master.

boocmp requested a review from atuchin-m August 11, 2025 12:20

boocmp self-assigned this Aug 11, 2025

boocmp force-pushed the flat_containers branch from 09b3365 to 6ed9f3a Compare August 11, 2025 12:22

github-actions bot reviewed Aug 11, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

boocmp marked this pull request as ready for review August 11, 2025 13:40

boocmp requested a review from antonok-edm as a code owner August 11, 2025 13:40

atuchin-m reviewed Aug 11, 2025

View reviewed changes

boocmp requested a review from atuchin-m August 12, 2025 10:27

boocmp force-pushed the flat_containers branch from a5d95ab to 49feb44 Compare August 12, 2025 11:07

atuchin-m reviewed Aug 12, 2025

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

atuchin-m reviewed Aug 12, 2025

View reviewed changes

src/flatbuffers/containers/sorted_index.rs Show resolved Hide resolved

atuchin-m requested a review from Copilot August 12, 2025 18:27

Copilot AI reviewed Aug 12, 2025

View reviewed changes

antonok-edm approved these changes Aug 13, 2025

View reviewed changes

boocmp added 13 commits August 13, 2025 14:31

Unsafe tools moved to flatbuffers.

d486797

Added FlatMultiMapView, FlatFilterMap replaced.

7a8ee9e

Added FlatSetView.

54482dd

Removed unused utils.

c8975dc

Temporary marked FlatSet as dead_code.

aab4940

Added test for FlatSetView.

06eda23

Added FlatMultiMapView tests.

5be314f

Clean up.

4c0f0ed

Ignore live test.

9be3ad4

Clippy fixes.

b94b56c

Review issues are addressed.

c4bbdee

Optional iterator for multimap (improve perf).

e1c705a

Removed unused file.

82b6f4f

boocmp force-pushed the flat_containers branch from 49feb44 to 82b6f4f Compare August 13, 2025 07:32

atuchin-m approved these changes Aug 13, 2025

View reviewed changes

atuchin-m merged commit 45b0371 into 0.11.x Aug 13, 2025
7 checks passed

atuchin-m deleted the flat_containers branch August 13, 2025 07:46

atuchin-m mentioned this pull request Aug 14, 2025

[Epic] Convert cosmetic filters to flat buffer #509

Open

	keys: self.keys.clone(), // Cloning is 3-4% faster than & in benchmarks
	keys: self.keys.clone(), // Clone keys for iterator; see implementation details for performance considerations

	partition_point_fbvector(self, \|x\| *x < key)
	fn lower_bound(&self, key: &str) -> usize {
	partition_point_fbvector(self, \|x\| x < key)

	partition_point_fbvector(self, \|x\| *x < key)
	partition_point_fbvector(self, \|x\| x < *key)

Flat containers #498

Flat containers #498

Uh oh!

Conversation

boocmp commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Rust Benchmark

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

antonok-edm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antonok-edm Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

atuchin-m Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

antonok-edm Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

atuchin-m Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

boocmp commented Aug 11, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading