Add a global RegexManager #240

atuchin-m · 2022-12-08T15:38:39Z

The idea is to use global regular expression storage instead of individual ones.
This allows to challenge the long-term regexp memory consumption problem in the next PRs.
A new RegexManager is in charge of matching/spawning/deleting regex instances. NetworkFilter is enforced to match using RegexManager.

Plans:

track the last regex usage and drop the instance it after a few time;
use less effective, but more memory-saving algorithms for some of the reg exps until it becomes a hot path;

Note:

Using a standard hasher for u64 instead of AHasher results in 3% performance drop.. Not sure AHasher is the best option. I am open to other suggestions.
Replacing a standard hasher in other places could also give a perf boost, worst trying.
get_debug_info() method is supposed to be exposed to something like brave://adblock-internals.

antonok-edm

I think overall this approach is a good idea.

I noticed a few places where the indentation is a bit messed up. I'd recommend cargo fmt, but currently it would produce a bunch of unrelated changes, which is not ideal. Could you at least go over the parts you've touched and make sure the indentation is 4 spaces? If it's easier you can copy your code changes into another file and format that one specifically.

src/filters/network.rs

antonok-edm · 2022-12-15T21:19:05Z

src/filters/network.rs

+
+    #[cfg(test)]
+    fn matches_test(&self, request: &request::Request) -> bool;


In the interest of not making breaking changes until the next semver bump, let's leave the matches and get_regex signatures untouched. We can add matches_with_regex_manager in addition to these.

get_regex can re-compile the regex whenever it's called.

matches could just be a thin wrapper around matches_with_regex_manager with an empty RegexManager; defined in-line with a #[deprecated] attribute.

That way, we don't need any #[cfg(test)] methods, and tests wouldn't have to be modified at all either.

The idea was to force all the instances to use a correct RegexManager (except the tests).
The thing that I'm afraid that we occasionally call the wrong function in the production code.
Like returning in old signature fn matches(&self, request: &request::Request) -> bool results in still usin it in some part of adblock-rust or 3rd party product.

That's why I believe we'd better to make breaking changes here.
A singleton as alternative doesn't sounds good..

Cargo.toml

src/blocker.rs

atuchin-m · 2022-12-28T13:31:24Z

@antonok-edm PTAL

atuchin-m · 2022-12-28T13:38:40Z

src/filters/network.rs

@@ -3283,17 +3329,11 @@ mod match_tests {
        {
            let filter = r#"/^https?:\/\/([0-9a-z\-]+\.)?(9anime|animeland|animenova|animeplus|animetoon|animewow|gamestorrent|goodanime|gogoanime|igg-games|kimcartoon|memecenter|readcomiconline|toonget|toonova|watchcartoononline)\.[a-z]{2,4}\/(?!([Ee]xternal|[Ii]mages|[Ss]cripts|[Uu]ploads|ac|ajax|assets|combined|content|cov|cover|(img\/bg)|(img\/icon)|inc|jwplayer|player|playlist-cat-rss|static|thumbs|wp-content|wp-includes)\/)(.*)/$image,other,script,~third-party,xmlhttprequest,domain=~animeland.hu"#;
            let network_filter = NetworkFilter::parse(filter, true, Default::default()).unwrap();
-            let regex = Arc::try_unwrap(network_filter.get_regex()).unwrap();
-            assert!(


Removed because there is no way to get the CompiledRegex type.
Do it important here to check it?

This test is just checking that the filter has a compiled regex. The equivalent would be checking that an external RegexManager compiles the regex for it (after a matches call). I don't think it's critical in this test, but we probably should have at least one test somewhere to confirm that the regex count increases as expected.

ok, I see.
I've modified the test to check get_compiled_regex_count() before/after calling is_match.

antonok-edm · 2023-01-06T03:32:31Z

src/filters/network.rs

+        for (_, v) in &self.map {
+          println!("{}", v.regex)
+        }


I assume we don't want this in production code; I'll remove it

Thanks for the correction.
Originally, that code was supposed to be in the next PR, but after the plan was changed.
So I messed up the branches and some test code went here.

Now I've changed the solution to preserve the map entries but to drop the regexs themselves.
This gives extra information about which reg expressions were discarded.

antonok-edm · 2023-01-06T03:33:58Z

src/filters/network.rs

+        self.map.clear();
+        let now = self.now;
+        self.map.retain(|_, v| now - v.last_used < std::time::Duration::from_secs(180));


self.map.clear() will completely empty the hashmap before the retain operation, which ends up being a no-op. I assume we just want the retain, so I removed the clear.

antonok-edm

I think it would be good to break RegexManager out into its own file; just another top-level regex_manager.rs would be fine. If you can do that and add a test or two there, I think it should be good to go.

antonok-edm · 2023-01-06T04:36:06Z

src/filters/network.rs

@@ -3283,17 +3329,11 @@ mod match_tests {
        {
            let filter = r#"/^https?:\/\/([0-9a-z\-]+\.)?(9anime|animeland|animenova|animeplus|animetoon|animewow|gamestorrent|goodanime|gogoanime|igg-games|kimcartoon|memecenter|readcomiconline|toonget|toonova|watchcartoononline)\.[a-z]{2,4}\/(?!([Ee]xternal|[Ii]mages|[Ss]cripts|[Uu]ploads|ac|ajax|assets|combined|content|cov|cover|(img\/bg)|(img\/icon)|inc|jwplayer|player|playlist-cat-rss|static|thumbs|wp-content|wp-includes)\/)(.*)/$image,other,script,~third-party,xmlhttprequest,domain=~animeland.hu"#;
            let network_filter = NetworkFilter::parse(filter, true, Default::default()).unwrap();
-            let regex = Arc::try_unwrap(network_filter.get_regex()).unwrap();
-            assert!(


This test is just checking that the filter has a compiled regex. The equivalent would be checking that an external RegexManager compiles the regex for it (after a matches call). I don't think it's critical in this test, but we probably should have at least one test somewhere to confirm that the regex count increases as expected.

atuchin-m · 2023-01-09T14:29:18Z

src/filters/network.rs

@@ -151,13 +152,15 @@ impl From<&request::RequestType> for NetworkFilterMask {
 pub enum CompiledRegex {
    Compiled(Regex),
    CompiledSet(RegexSet),
+    None,


I've added this new entry instead of using Option<CompiledRegex>.
This allow to avoid growing the map entry to +8 bytes (because of extra Option).

Motivation: https://adeschamps.github.io/enum-size

atuchin-m · 2023-01-09T14:33:44Z

Moved RegexManager to regex_manager.rs .

antonok-edm · 2023-01-10T02:21:43Z

src/filters/network.rs

    MatchAll,
    RegexParsingError(regex::Error),
 }

 impl CompiledRegex {
    pub fn is_match(&self, pattern: &str) -> bool {
        match &self {
+            CompiledRegex::None => {panic!("use of CompiledRegex::None")} // No value, invalid call.


This is the kind of thing I'd prefer to see behind the debug-info flag, so we can guarantee it won't cause crashes for users in the wild.

Can we do this?

Rename CompiledRegex::None to CompiledRegex::Discarded

Put CompiledRegex::Discarded behind a #[cfg(feature = "debug-info")]

Completely remove the CompiledRegex in cleanup when debug-info is disabled

In theory we can, I also though about the similar approach.
But I believe we need discarded map entries to make a better decision in future (=stop discarding the entries that we often recreate.

But I believe we need discarded map entries to make a better decision in future (=stop discarding the entries that we often recreate.

I think that makes sense as an approach, but I'd strongly prefer to add panics like this only when necessary. Unless you're planning to implement that strategy in this PR, let's leave it out and we can revisit later.

There could also be alternatives to this pattern that wouldn't panic (maybe we can store hashes of the discarded rules in a bloom filter or hashmap?). But we should look at that holistically.

Changed to Option<CompiledRegex>.

src/filters/regex_manager.rs

atuchin-m · 2023-01-11T15:57:22Z

@antonok-edm Any other comments?

antonok-edm

last few comments and it should be good to go

src/filters/regex_manager.rs

Cargo.toml

atuchin-m · 2023-01-16T08:18:28Z

@antonok-edm PTAL

Draft to implement the storage Try to optimize Use AHasher Clean up and stats Rename and cleanup

atuchin-m requested a review from antonok-edm December 8, 2022 15:38

atuchin-m self-assigned this Dec 8, 2022

atuchin-m force-pushed the use-global-regex-storage branch from 63b2697 to b9f217d Compare December 9, 2022 09:28

antonok-edm reviewed Dec 15, 2022

View reviewed changes

atuchin-m commented Dec 28, 2022

View reviewed changes

antonok-edm reviewed Jan 6, 2023

View reviewed changes

antonok-edm force-pushed the use-global-regex-storage branch 2 times, most recently from 63c66d5 to ee1978d Compare January 6, 2023 04:08

antonok-edm reviewed Jan 6, 2023

View reviewed changes

atuchin-m commented Jan 9, 2023

View reviewed changes

antonok-edm reviewed Jan 10, 2023

View reviewed changes

antonok-edm reviewed Jan 13, 2023

View reviewed changes

src/filters/regex_manager.rs Outdated Show resolved Hide resolved

src/filters/regex_manager.rs Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

atuchin-m and others added 14 commits January 16, 2023 00:54

Pass a global regexp via methods

b989c90

Draft to implement the storage Try to optimize Use AHasher Clean up and stats Rename and cleanup

Time based retain logic

2854785

cargo fmt

87caac5

Return unsync-regex-caching

0e239a3

add debug-info flag, remove ahash

9c9209a

Move RegexManager to a dedicated file

2aa0502

Save discarded map's entries

355a004

Add regex_manager tests

a9e8c2c

Remove CompiledRegex::None

161957c

cargo fmt

bbedfd1

Fix debug-info

4f07e2f

Fix compilation for non-test

8f2a586

Fix reviews issues

2f390a2

minor fixups

952df88

antonok-edm force-pushed the use-global-regex-storage branch from f32a785 to 952df88 Compare January 16, 2023 09:09

antonok-edm merged commit 066eb36 into master Jan 16, 2023

antonok-edm deleted the use-global-regex-storage branch January 16, 2023 09:09

atuchin-m mentioned this pull request Jan 23, 2023

Introduce brave://adblock-internals brave/brave-core#16797

Merged

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a global RegexManager #240

Add a global RegexManager #240

atuchin-m commented Dec 8, 2022 •

edited

Loading

antonok-edm left a comment

antonok-edm Dec 15, 2022

atuchin-m Dec 16, 2022

atuchin-m commented Dec 28, 2022

atuchin-m Dec 28, 2022

antonok-edm Jan 6, 2023

atuchin-m Jan 9, 2023

antonok-edm Jan 6, 2023

atuchin-m Jan 9, 2023

antonok-edm Jan 6, 2023

antonok-edm left a comment

antonok-edm Jan 6, 2023

atuchin-m Jan 9, 2023

atuchin-m commented Jan 9, 2023

antonok-edm Jan 10, 2023

atuchin-m Jan 10, 2023

antonok-edm Jan 11, 2023

atuchin-m Jan 12, 2023

atuchin-m commented Jan 11, 2023

antonok-edm left a comment

atuchin-m commented Jan 16, 2023


		#[cfg(test)]
		fn matches_test(&self, request: &request::Request) -> bool;

Add a global RegexManager #240

Add a global RegexManager #240

Conversation

atuchin-m commented Dec 8, 2022 • edited Loading

antonok-edm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atuchin-m commented Dec 28, 2022

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

antonok-edm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atuchin-m commented Jan 9, 2023

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

atuchin-m commented Jan 11, 2023

antonok-edm left a comment

Choose a reason for hiding this comment

atuchin-m commented Jan 16, 2023

atuchin-m commented Dec 8, 2022 •

edited

Loading