Add caching of graphs for ABAC filtering - #2800
Draft
arienandalibi wants to merge 6 commits into
Draft
arienandalibi wants to merge 6 commits into
arienandalibi wants to merge 6 commits into
Conversation
…, making sure to cache denials as well (except for cache config errors). The key is carefully selected to only include what's relevant. Added tests for this. We also add a MutationListener to notify the authorization policy when changes to any graph are made so that we can clear its cache. We track which claims are accessed by each role/probe/filter. We track the WritableStore's generation as well, just an index which increments whenever we make a modification to it.
… performance when the source graph won't be mutated much. We now keep all roles as a set because we can't merge individual filters anymore. General cleanup. Update tests. Added static contributions (roles that don't need probes), we track their associated claims like the others. refine_permission now may return a cached graph. Fixed support for the static layer showing unlayered nodes in CachedView.
… that don't read any claims (or probes), add tests for default edge layer behaviour
# Conflicts: # raphtory-graphql/src/data.rs # raphtory-graphql/src/graph.rs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
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 2.
| Benchmark suite | Current: a10af46 | Previous: 9823ef7 | Ratio |
|---|---|---|---|
lotr_graph/num_edges |
5 ns/iter (± 0) |
0 ns/iter (± 0) |
+∞ |
lotr_graph/num_nodes |
4 ns/iter (± 0) |
1 ns/iter (± 0) |
4 |
lotr_graph/graph_latest |
3 ns/iter (± 0) |
0 ns/iter (± 0) |
+∞ |
lotr_graph_materialise/materialize |
8005251 ns/iter (± 52498) |
1564816 ns/iter (± 35303) |
5.12 |
lotr_graph_window_100/num_nodes |
15 ns/iter (± 0) |
5 ns/iter (± 0) |
3 |
lotr_graph_window_100_materialise/materialize |
7969620 ns/iter (± 75593) |
1669150 ns/iter (± 10700) |
4.77 |
lotr_graph_window_10/has_node_existing |
128 ns/iter (± 7) |
62 ns/iter (± 11) |
2.06 |
lotr_graph_window_10_materialise/materialize |
3380769 ns/iter (± 72224) |
971980 ns/iter (± 4278) |
3.48 |
lotr_graph_subgraph_10pc/num_nodes |
16 ns/iter (± 0) |
4 ns/iter (± 0) |
4 |
lotr_graph_subgraph_10pc_materialise/materialize |
2238434 ns/iter (± 27446) |
334634 ns/iter (± 1287) |
6.69 |
lotr_graph_subgraph_10pc_windowed/has_node_existing |
131 ns/iter (± 8) |
62 ns/iter (± 14) |
2.11 |
lotr_graph_subgraph_10pc_windowed_materialise/materialize |
1371740 ns/iter (± 24285) |
230399 ns/iter (± 2617) |
5.95 |
lotr_graph_window_50_layered/has_node_existing |
398 ns/iter (± 33) |
129 ns/iter (± 12) |
3.09 |
lotr_graph_window_50_layered/graph_latest |
75209 ns/iter (± 781) |
36649 ns/iter (± 916) |
2.05 |
lotr_graph_window_50_layered_materialise/materialize |
28131285 ns/iter (± 256538) |
3488825 ns/iter (± 24948) |
8.06 |
lotr_graph_persistent_window_50_layered/num_edges_temporal |
554914 ns/iter (± 7869) |
192686 ns/iter (± 1569) |
2.88 |
lotr_graph_persistent_window_50_layered/has_node_existing |
419 ns/iter (± 359) |
174 ns/iter (± 83) |
2.41 |
lotr_graph_persistent_window_50_layered/iterate_exploded_edges |
3336948 ns/iter (± 96851) |
1659940 ns/iter (± 19402) |
2.01 |
lotr_graph_persistent_window_50_layered_materialise/materialize |
47816213 ns/iter (± 459173) |
5298035 ns/iter (± 147912) |
9.03 |
This comment was automatically generated by workflow using github-action-benchmark.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
To mitigate overhead associated with ABAC permissions and filtering of the source graph based on any individual access permissions, we add caching of accessed graphs. When a user accesses the same graph with the same access permissions, we use the cached graph. This skips the generation of the filter itself (which may require running some probes) as well as the filtering of the graph (since the graph itself was cached). The first implementation is naive, and any modification to any graph on the server as well as any modification to the permissions store clears all caches.
Why are the changes needed?
Performance improvements.
Does this PR introduce any user-facing change? If yes is this documented?
No, the changes should not be user-facing. All changes should be internal.
How was this patch tested?
Unit tests and integration tests.
Are there any further changes required?
To be determined. We may want to implement smarter cache invalidation, so that the entire cache isn't cleared on any mutation to any graph. We can invalidate only the affected graphs on mutations.