feat(execution-context): reset() to reuse one context across inputs - #26
Merged
Merged
Conversation
An ExecutionContext holds one value slot per scheme field, and
allocating and zeroing that table dominates the cost of filling one:
for an 84-field scheme it is ~300 ns, more than every field write
combined and paid before the first one.
clear() already keeps the allocation, but it cannot help a caller in a
loop: the context's 'e is fixed to the data it was last filled from, so
the next input -- borrowed from somewhere else -- does not typecheck
against it. Callers in that shape (one context per packet, per request,
per row) therefore allocate a fresh context every time.
reset() clears the context and re-labels it for a new borrow lifetime,
so the allocation can be handed from input to input:
let mut spare = ExecutionContext::new(&scheme);
for input in inputs {
let mut ctx = spare.reset();
ctx.set_field_value(field, &input.value)?;
// ... execute filters ...
spare = ctx.reset();
}
Sound because clear() has already dropped every LhsValue<'e> and the
remaining members do not borrow from 'e; the two types differ only in
that lifetime parameter. Covered by a test that borrows from a value
which dies before the next iteration -- the case that would be UB if
the re-labelling were wrong -- and it passes under Miri with
-Zmiri-strict-provenance.
byte_char_slices on the wildcard-replace flag test and the truncate(0)-instead-of-clear in the panic hook. Both are new lints on current stable, both fail the -D warnings gate on any PR, and neither changes behaviour.
tombi format --check has been failing on this file for every PR: it writes target table keys with double quotes. Cargo accepts either form and the dependency graph is unchanged (cargo metadata and a wirefilter-ffi check both still resolve the cfg(unix) dev-dependency).
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
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.
Why
An
ExecutionContextholds one value slot per scheme field, and allocatingand zeroing that table dominates the cost of filling one. Measured on an
84-field scheme:
ExecutionContext::newclear()on an existing oneThat 316 ns is more than every field write in a typical bind combined, and it
is paid before the first one.
clear()already keeps the allocation, but it cannot help a caller in a loop:the context's
'eis fixed to the data it was last filled from, so the nextinput — borrowed from somewhere else — will not typecheck against it. Callers
in that shape (one context per packet, per request, per row) end up allocating
a fresh context every time.
What
reset()clears the context and re-labels it for a new borrow lifetime, soone allocation can be handed from input to input:
In the caller this PR was written for — a per-packet firewall bind over an
84-field scheme — this takes binding a packet from 616 ns to 439 ns (−29%)
with no other change.
Soundness
clear()has already replaced every value withNone, dropping eachLhsValue<'e>, and cleared the list matchers. The remaining members do notborrow from
'e:schemeis owned,list_matchersare'statictraitobjects, and
user_dataisU. With nothing reachable that borrows from'e, re-labelling the lifetime cannot produce a dangling reference, and thetwo types differ only in that parameter, so their layout is identical.
test_reset_reuses_one_context_across_borrowsexercises the case that wouldbe UB if this were wrong — each iteration borrows from a
Stringthat isdropped before the next iteration begins — and it passes under Miri with
-Zmiri-strict-provenance.