Allow custom resource storage backends, expose more debug info #526
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: 21ffdbb | Previous: 1bfadd6 | Ratio |
|---|---|---|---|
rule-match-browserlike/brave-list |
2181878759 ns/iter (± 21050820) |
2157327098 ns/iter (± 15833464) |
1.01 |
rule-match-first-request/brave-list |
1107660 ns/iter (± 20651) |
1067208 ns/iter (± 8765) |
1.04 |
blocker_new/brave-list |
146930661 ns/iter (± 549604) |
175128732 ns/iter (± 2762769) |
0.84 |
blocker_new/brave-list-deserialize |
20680462 ns/iter (± 98755) |
73769476 ns/iter (± 854420) |
0.28 |
memory-usage/brave-list-initial |
10072539 ns/iter (± 3) |
18344503 ns/iter (± 3) |
0.55 |
memory-usage/brave-list-initial/max |
64817658 ns/iter (± 3) |
66961277 ns/iter (± 3) |
0.97 |
memory-usage/brave-list-initial/alloc-count |
1534723 ns/iter (± 3) |
1616082 ns/iter (± 3) |
0.95 |
memory-usage/brave-list-1000-requests |
2516487 ns/iter (± 3) |
2551890 ns/iter (± 3) |
0.99 |
memory-usage/brave-list-1000-requests/alloc-count |
66788 ns/iter (± 3) |
68816 ns/iter (± 3) |
0.97 |
url_cosmetic_resources/brave-list |
201939 ns/iter (± 1686) |
189708 ns/iter (± 2713) |
1.06 |
cosmetic-class-id-match/brave-list |
15931994 ns/iter (± 4829173) |
5166097 ns/iter (± 1523809) |
3.08 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Rust Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.
| Benchmark suite | Current: 21ffdbb | Previous: 1bfadd6 | Ratio |
|---|---|---|---|
cosmetic-class-id-match/brave-list |
15931994 ns/iter (± 4829173) |
5166097 ns/iter (± 1523809) |
3.08 |
This comment was automatically generated by workflow using github-action-benchmark.
antonok-edm
force-pushed
the
adjust-engine-public-api-2
branch
2 times, most recently
from
7a4eed6 to
e454c54
Compare
| - name: Checkout | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
|
|
||
| # Can be removed once https://github.com/actions/runner-images/pull/13076 is released |
There was a problem hiding this comment.
Let's remove this to make the diff clear.
I've pushed the recent fixes from master to 0.11.x
| // Since the beginning of flatbuffer data is aligned to that size, | ||
| // the alignment of the data must be a divisor of MIN_ALIGNMENT. | ||
| assert!(MIN_ALIGNMENT % std::mem::size_of::<T>() == 0); | ||
| assert!(MIN_ALIGNMENT.is_multiple_of(std::mem::size_of::<T>())); |
There was a problem hiding this comment.
the same, let's revert unrelated changes.
| /// a custom [ResourceStorageBackend] if desired. | ||
| pub struct ResourceStorage { | ||
| #[cfg(not(feature = "single-thread"))] | ||
| backend: Box<dyn ResourceStorageBackend + Sync + Send>, |
There was a problem hiding this comment.
hm, I'm not strongly again Box<dyn ..> it makes the code less clear and may have some negative perf effect (if it's a hot path)
| } | ||
| } | ||
|
|
||
| impl InMemoryResourceStorage { |
There was a problem hiding this comment.
That code have major conflicts with master with #517.
| /// Customizable backend for [Resource] storage. | ||
| /// Custom implementations could be used to enable (for example) sharing of resources between | ||
| /// multiple [crate::Engine]s, an on-disk backend, or special caching behavior. | ||
| pub trait ResourceStorageBackend { |
There was a problem hiding this comment.
That trait looks internal to me. Can we don't make it public? I would say the best option is just to provide a few public methods to build ResourceStorage without exposing the other struct/traits
|
Frankly, I can’t find the part that allow to shared the resources between multiple engines (the idea of my #519). |
antonok-edm
force-pushed
the
adjust-engine-public-api-2
branch
from
ccebbee to
eb55494
Compare
atuchin-m
left a comment
atuchin-m
left a comment
There was a problem hiding this comment.
LGTM to merge after rebasing to the recent 0.11.x changes.
unfortunately this requires duplicating some definitions to support additional `Send + Sync` trait bounds, since Rust does not natively support conditional supertraits.
antonok-edm
force-pushed
the
adjust-engine-public-api-2
branch
from
eb55494 to
21ffdbb
Compare
supersedes #519
breaking API changes from this PR:
ResourceStorageBackend. This trait can be implemented to use customized storage backend logic, including sharing between multiple engines, caching from disk, ability to dynamically add/remove resources at runtime, collecting metrics, etc.InMemoryResourceStorageas a port of the originalResourceStoragebackend.Engine::add_resource. If you still need the ability to add individual resources at runtime, use a customResourceStorageBackendinstead.regex-debug-infofeature has been named todebug-info, as it now also enables a new API to get the size of theEngine's internal flatbuffer at runtime.