Merge pull request #434 from brave/code_structure_4

Code structure follow up #3.

Author: boocmp

Cargo.lock

@@ -36,7 +36,6 @@ seahash = "3"   # seahash 4 introduces a breaking hash algorithm change
 memchr = "2.4"
 base64 = "0.13"
 rmp-serde = "0.15"
-lifeguard = { version = "^ 0.6.1", optional = true }
 cssparser = { version = "0.29", optional = true }
 selectors = { version = "0.24", optional = true }
 serde_json = "1.0"
@@ -89,9 +88,8 @@ harness = false
 [features]
 # If disabling default features, consider explicitly re-enabling the
 # "embedded-domain-resolver" feature.
-default = ["embedded-domain-resolver", "full-regex-handling", "object-pooling", "unsync-regex-caching"]
+default = ["embedded-domain-resolver", "full-regex-handling", "unsync-regex-caching"]
 full-regex-handling = []
-object-pooling = ["lifeguard"] # disables `Send` and `Sync` on `Engine`.
 unsync-regex-caching = [] # disables `Send` and `Sync` on `Engine`.
 regex-debug-info = []
 css-validation = ["cssparser", "selectors"]

Cargo.toml

@@ -52,7 +52,7 @@ By default, `adblock-rust` ships with a built-in domain resolution implementatio
 `adblock-rust` uses uBlock Origin-compatible resources for scriptlet injection and redirect rules.
 The `resource-assembler` feature allows `adblock-rust` to parse these resources directly from the file formats used by the uBlock Origin repository.
 
-#### Thread safety (`object-pooling`, `unsync-regex-caching`)
+#### Thread safety (`unsync-regex-caching`)
 
-The `object-pooling` and `unsync-regex-caching` features enable optimizations for rule matching speed and the amount of memory used by the engine.
-These features can be disabled to make the engine `Send + Sync`, although it is recommended to only access the engine on a single thread to maintain optimal performance.
+The `unsync-regex-caching` feature enables optimizations for rule matching speed and the amount of memory used by the engine.
+This feature can be disabled to make the engine `Send + Sync`, although it is recommended to only access the engine on a single thread to maintain optimal performance.

README.md

@@ -8,15 +8,11 @@ use std::ops::DerefMut;
 use std::sync::Arc;
 use thiserror::Error;
 
-#[cfg(feature = "object-pooling")]
-use lifeguard::Pool;
-
 use crate::filters::network::{NetworkFilter, NetworkFilterMaskHelper, NetworkMatchable};
 use crate::optimizer;
 use crate::regex_manager::{RegexManager, RegexManagerDiscardPolicy};
 use crate::request::Request;
 use crate::resources::ResourceStorage;
-use crate::utils;
 use crate::utils::{fast_hash, Hash};
 
 /// Options used when constructing a [`Blocker`].
@@ -88,25 +84,6 @@ pub enum BlockerError {
     FilterExists,
 }
 
-#[cfg(feature = "object-pooling")]
-pub(crate) struct TokenPool {
-    pub pool: Pool<Vec<utils::Hash>>,
-}
-
-#[cfg(feature = "object-pooling")]
-impl Default for TokenPool {
-    fn default() -> TokenPool {
-        TokenPool {
-            pool: lifeguard::pool()
-                .with(lifeguard::StartingSize(1))
-                .with(lifeguard::Supplier(|| {
-                    Vec::with_capacity(utils::TOKENS_BUFFER_SIZE)
-                }))
-                .build(),
-        }
-    }
-}
-
 // only check for tags in tagged and exception rule buckets,
 // pass empty set for the rest
 static NO_TAGS: Lazy<HashSet<String>> = Lazy::new(HashSet::new);
@@ -129,10 +106,6 @@ pub struct Blocker {
 
     pub(crate) enable_optimizations: bool,
 
-    // Not serialized
-    #[cfg(feature = "object-pooling")]
-    pub(crate) pool: TokenPool,
-
     // Not serialized
     #[cfg(feature = "unsync-regex-caching")]
     pub(crate) regex_manager: std::cell::RefCell<RegexManager>,
@@ -167,24 +140,8 @@ impl Blocker {
 
     pub fn check_generic_hide(&self, hostname_request: &Request) -> bool {
         let mut regex_manager = self.borrow_regex_manager();
-        let mut request_tokens;
-        #[cfg(feature = "object-pooling")]
-        {
-            request_tokens = self.pool.pool.new();
-        }
-        #[cfg(not(feature = "object-pooling"))]
-        {
-            request_tokens = Vec::with_capacity(utils::TOKENS_BUFFER_SIZE);
-        }
-        hostname_request.get_tokens(&mut request_tokens);
-
         self.generic_hide
-            .check(
-                hostname_request,
-                &request_tokens,
-                &HashSet::new(),
-                &mut regex_manager,
-            )
+            .check(hostname_request, &HashSet::new(), &mut regex_manager)
             .is_some()
     }
 
@@ -200,70 +157,41 @@ impl Blocker {
             return BlockerResult::default();
         }
 
-        let mut request_tokens;
-        #[cfg(feature = "object-pooling")]
-        {
-            request_tokens = self.pool.pool.new();
-        }
-        #[cfg(not(feature = "object-pooling"))]
-        {
-            request_tokens = Vec::with_capacity(utils::TOKENS_BUFFER_SIZE);
-        }
-        request.get_tokens(&mut request_tokens);
-
         // Check the filters in the following order:
         // 1. $important (not subject to exceptions)
         // 2. redirection ($redirect=resource)
         // 3. normal filters - if no match by then
         // 4. exceptions - if any non-important match of forced
 
         // Always check important filters
-        let important_filter =
-            self.importants
-                .check(request, &request_tokens, &NO_TAGS, &mut regex_manager);
+        let important_filter = self.importants.check(request, &NO_TAGS, &mut regex_manager);
 
         // only check the rest of the rules if not previously matched
         let filter = if important_filter.is_none() && !matched_rule {
             self.filters_tagged
-                .check(
-                    request,
-                    &request_tokens,
-                    &self.tags_enabled,
-                    &mut regex_manager,
-                )
-                .or_else(|| {
-                    self.filters
-                        .check(request, &request_tokens, &NO_TAGS, &mut regex_manager)
-                })
+                .check(request, &self.tags_enabled, &mut regex_manager)
+                .or_else(|| self.filters.check(request, &NO_TAGS, &mut regex_manager))
         } else {
             important_filter
         };
 
         let exception = match filter.as_ref() {
             // if no other rule matches, only check exceptions if forced to
-            None if matched_rule || force_check_exceptions => self.exceptions.check(
-                request,
-                &request_tokens,
-                &self.tags_enabled,
-                &mut regex_manager,
-            ),
+            None if matched_rule || force_check_exceptions => {
+                self.exceptions
+                    .check(request, &self.tags_enabled, &mut regex_manager)
+            }
             None => None,
             // If matched an important filter, exceptions don't atter
             Some(f) if f.is_important() => None,
-            Some(_) => self.exceptions.check(
-                request,
-                &request_tokens,
-                &self.tags_enabled,
-                &mut regex_manager,
-            ),
+            Some(_) => self
+                .exceptions
+                .check(request, &self.tags_enabled, &mut regex_manager),
         };
 
-        let redirect_filters = self.redirects.check_all(
-            request,
-            &request_tokens,
-            &NO_TAGS,
-            regex_manager.deref_mut(),
-        );
+        let redirect_filters =
+            self.redirects
+                .check_all(request, &NO_TAGS, regex_manager.deref_mut());
 
         // Extract the highest priority redirect directive.
         // 1. Exceptions - can bail immediately if found
@@ -328,12 +256,7 @@ impl Blocker {
         let rewritten_url = if important {
             None
         } else {
-            Self::apply_removeparam(
-                &self.removeparam,
-                request,
-                &request_tokens,
-                regex_manager.deref_mut(),
-            )
+            Self::apply_removeparam(&self.removeparam, request, regex_manager.deref_mut())
         };
 
         // If something has already matched before but we don't know what, still return a match
@@ -351,7 +274,6 @@ impl Blocker {
     fn apply_removeparam(
         removeparam_filters: &NetworkFilterList,
         request: &Request,
-        request_tokens: &[Hash],
         regex_manager: &mut RegexManager,
     ) -> Option<String> {
         /// Represents an `&`-separated argument from a URL query parameter string
@@ -395,8 +317,7 @@ impl Blocker {
                 .map(|param| (param, true))
                 .collect();
 
-            let filters =
-                removeparam_filters.check_all(request, request_tokens, &NO_TAGS, regex_manager);
+            let filters = removeparam_filters.check_all(request, &NO_TAGS, regex_manager);
             let mut rewrite = false;
             for removeparam_filter in filters {
                 if let Some(removeparam) = &removeparam_filter.modifier_option {
@@ -448,25 +369,10 @@ impl Blocker {
             return None;
         }
 
-        let mut request_tokens;
         let mut regex_manager = self.borrow_regex_manager();
-
-        #[cfg(feature = "object-pooling")]
-        {
-            request_tokens = self.pool.pool.new();
-        }
-        #[cfg(not(feature = "object-pooling"))]
-        {
-            request_tokens = Vec::with_capacity(utils::TOKENS_BUFFER_SIZE);
-        }
-        request.get_tokens(&mut request_tokens);
-
-        let filters = self.csp.check_all(
-            request,
-            &request_tokens,
-            &self.tags_enabled,
-            &mut regex_manager,
-        );
+        let filters = self
+            .csp
+            .check_all(request, &self.tags_enabled, &mut regex_manager);
 
         if filters.is_empty() {
             return None;
@@ -597,9 +503,6 @@ impl Blocker {
             tagged_filters_all,
             // Options
             enable_optimizations: options.enable_optimizations,
-
-            #[cfg(feature = "object-pooling")]
-            pool: TokenPool::default(),
             regex_manager: Default::default(),
         }
     }
@@ -888,34 +791,14 @@ impl NetworkFilterList {
     pub fn check(
         &self,
         request: &Request,
-        request_tokens: &[Hash],
         active_tags: &HashSet<String>,
         regex_manager: &mut RegexManager,
     ) -> Option<&NetworkFilter> {
         if self.filter_map.is_empty() {
             return None;
         }
 
-        if let Some(source_hostname_hashes) = request.source_hostname_hashes.as_ref() {
-            for token in source_hostname_hashes {
-                if let Some(filter_bucket) = self.filter_map.get(token) {
-                    for filter in filter_bucket {
-                        // if matched, also needs to be tagged with an active tag (or not tagged at all)
-                        if filter.matches(request, regex_manager)
-                            && filter
-                                .tag
-                                .as_ref()
-                                .map(|t| active_tags.contains(t))
-                                .unwrap_or(true)
-                        {
-                            return Some(filter);
-                        }
-                    }
-                }
-            }
-        }
-
-        for token in request_tokens {
+        for token in request.get_tokens_for_match() {
             if let Some(filter_bucket) = self.filter_map.get(token) {
                 for filter in filter_bucket {
                     // if matched, also needs to be tagged with an active tag (or not tagged at all)
@@ -942,7 +825,6 @@ impl NetworkFilterList {
     pub fn check_all(
         &self,
         request: &Request,
-        request_tokens: &[Hash],
         active_tags: &HashSet<String>,
         regex_manager: &mut RegexManager,
     ) -> Vec<&NetworkFilter> {
@@ -952,26 +834,7 @@ impl NetworkFilterList {
             return filters;
         }
 
-        if let Some(source_hostname_hashes) = request.source_hostname_hashes.as_ref() {
-            for token in source_hostname_hashes {
-                if let Some(filter_bucket) = self.filter_map.get(token) {
-                    for filter in filter_bucket {
-                        // if matched, also needs to be tagged with an active tag (or not tagged at all)
-                        if filter.matches(request, regex_manager)
-                            && filter
-                                .tag
-                                .as_ref()
-                                .map(|t| active_tags.contains(t))
-                                .unwrap_or(true)
-                        {
-                            filters.push(filter);
-                        }
-                    }
-                }
-            }
-        }
-
-        for token in request_tokens {
+        for token in request.get_tokens_for_match() {
             if let Some(filter_bucket) = self.filter_map.get(token) {
                 for filter in filter_bucket {
                     // if matched, also needs to be tagged with an active tag (or not tagged at all)
@@ -987,7 +850,6 @@ impl NetworkFilterList {
                 }
             }
         }
-
         filters
     }
 }

src/blocker.rs

@@ -494,9 +494,6 @@ impl From<DeserializeFormat> for (Blocker, CosmeticFilterCache) {
                 tagged_filters_all: v.tagged_filters_all.into_iter().map(|f| f.into()).collect(),
 
                 enable_optimizations: v.enable_optimizations,
-
-                #[cfg(feature = "object-pooling")]
-                pool: Default::default(),
                 regex_manager: Default::default(),
             },
             CosmeticFilterCache {

src/data_format/v0.rs

@@ -273,7 +273,7 @@ impl Engine {
 }
 
 /// Static assertions for `Engine: Send + Sync` traits.
-#[cfg(not(any(feature = "object-pooling", feature = "unsync-regex-caching")))]
+#[cfg(not(feature = "unsync-regex-caching"))]
 fn _assertions() {
     fn _assert_send<T: Send>() {}
     fn _assert_sync<T: Sync>() {}

src/engine.rs

@@ -134,7 +134,7 @@ where
         return true;
     }
     let request_url = request.get_url(mask.match_case());
-    filters.any(|f| &request_url == f)
+    filters.any(|f| request_url == f)
 }
 
 // pattern*^