Subcontext: replace ephemerals with a new scope with user-defined lifetime derived from the context#443
Conversation
28ea3ed to
17bb5a1
Compare
a525d83 to
875078a
Compare
Benchmarks clangBenchmark execution time: 2025-09-10 11:56:45 Comparing candidate commit c8d5afd in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 0 metrics, 0 unstable metrics. scenario:global-benchmark.random.clang
|
Benchmarks clang-pgoBenchmark execution time: 2025-09-10 12:11:54 Comparing candidate commit c8d5afd in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
8662c14 to
860dd72
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## anilm3/v2 #443 +/- ##
=============================================
- Coverage 85.04% 84.71% -0.33%
=============================================
Files 183 183
Lines 9554 9657 +103
Branches 4173 4211 +38
=============================================
+ Hits 8125 8181 +56
- Misses 572 607 +35
- Partials 857 869 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
860dd72 to
c0b389f
Compare
Artifact Size Comparison 📦
|
a059337 to
1870308
Compare
1870308 to
5df0237
Compare
d3634b3 to
4d3287b
Compare
…e within the evaluation engine
b8231e7 to
c4d3558
Compare
c4d3558 to
edad41a
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
8200cd6 to
e1eca4b
Compare
e1eca4b to
3fc9fc1
Compare
Anilm3
changed the title
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
Anilm3
changed the title
| return engine_->eval(deadline); | ||
| } | ||
|
|
||
| subcontext create_subcontext() { return subcontext{engine_, mr_}; } |
There was a problem hiding this comment.
I don't think the subcontext should use the same (monotonic) memory resource as the parent. If we have a long running application that, say, makes a bunch of http API calls using subcontexts, the memory could increase indefinitely, as it wouldn't be reclaimed upon subcontext destruction.
There was a problem hiding this comment.
The memory resource set in the thread local for the subcontext must be the same one as the parent or the existing structures may suddenly use two different memory resources, with potentially unknown consequences. However, I tried to avoid allocating anything with limited lifetime within the subcontext using this resource, if you find anything I can fix it.
| * | ||
| * @note The WAF instance needs to be valid for the lifetime of the subcontext. | ||
| **/ | ||
| ddwaf_subcontext ddwaf_subcontext_init(ddwaf_context context); |
There was a problem hiding this comment.
It looks like, for the purposes of thread safety, contexts and their subcontexts should be treated as the same object (more or less). These requirements are not documented.
There was a problem hiding this comment.
I'm going to make some changes in the next PR to forbid the simultaneous use of contexts and subcontexts, I'll update the remaining docs once I've done it.
|
|
||
| memory::list<owned_object> input_objects_; | ||
| std::list<owned_object> ephemeral_objects_; | ||
| std::list<owned_object> subcontext_objects_; |
There was a problem hiding this comment.
I think there are two viable/consistent options here (see my comment about the reuse of the memory resource from the parent ctx). I'm assuming that the subcontext, when running, should make no allocations affecting the parent:
- use a separate monotonic resource for the subcontext. The thread-local allocator would route the request to either the resource of the context or the active subrequests depending on the active scope. You would need to able to completely clear the
subcontext_objects_andsubcontext_targets_upon subcontext destruction (no preallocated arrays should remain). - keep using std::list / std::unordered_set. But the thread local allocator should refuse (abort() or at least log an error) if there were allocations to the
mr_during subcontext evaluation.
There was a problem hiding this comment.
I'll give it some thought...
There was a problem hiding this comment.
After some thought, I think it's fine for the subcontext to use the context allocator, as along as it doesn't involve subcontext specific caching (e.g. updating a fixed-size cache which could be created in either the context or subcontext).
I know there's a risk of misuse, without larger changes it may not be possible to address, but I'll keep it in mind.
eccc7a4 to
9eb5e4e
Compare
9eb5e4e to
c8d5afd
Compare
* Object view: read only abstraction to ddwaf_object (#341) * [v2] Remove mingw builds (#381) * Writable objects: owned and borrowed object and object limits removal (#378, #382) * [v2] Refactor and improve object types (#387) * [v2] Update unit tests to use new abstractions (#389) * [v2] Update `raw_configuration` type to use `object_view` (#390) * [v2] Remove remaining uses of ddwaf_object in `src` and `tests/unit` (#391) * [v2] JWT Decoding Processor (#401) * [v2] First iteration of object layout changes (#394) * Split context data insertion from evaluation (#407) * [v2] Second iteration of object layout changes (#408) * [v2] Add new fingerprint and object view tests (#414) * Reenable attribute collector unit test (#415) * [v2] Container view types (#413) * Exclude assertions from coverage (#416) * [v2] Use allocators internally instead of malloc/free and stop generating zero-terminated strings (#418) * Add memory resource to owned and borrowed objects (#428) * [v2] Propagate allocators from context (#420) * [v2] Update interface and expose allocators (#427) * Refactor evaluation stages out of the context (#442) * Subcontext: replace ephemerals with a new scope with user-defined lifetime derived from the context (#443) * Validator: Add support for testing subcontexts and attributes (#451) * [v2] Pass allocator to context and subcontext eval and add new allocators (#452) * Update logger to avoid dependencies on ddwaf.h (#453) * [v2] Return DDWAF_MATCH when there are events, attributes or actions (#455) * [v2] Cleanup: remove exclusion namespace and some redundant references (#456)
3 participants
This PR introduces the subcontext, new evaluation scope derived from the context with the goal of replacing ephemerals. The lifetime of an ephemeral is that of the call in which they are provided, in contrast, the lifetime of the data provided to the subcontext is that of the subcontext itself, in a similar manner to the data provided to the context.
A subcontext represents a short-lived scope to evaluate data which may be generated multiple times within a request, for example, a downstream request generates new data (url, method, body, query parameters, etc) which is not entirely linked to the main request and may need to be evaluated multiple times from different downstream requests, therefore a subcontext may be used to encompoass the lifetime of the downstream request.
At this stage, the subcontext implementation has been achieved by keeping track of the current evaluation scope (context or subcontext), where each subcontext is distinctly identified by a 32-bit ID, therefore cache invalidation is done through the comparison of the scope.
Also note that only one subcontext may exist at a time.
Pending: