Skip to content

perf: remove hot-path allocation, syscalls, and redundant fsyncs - #3

Open
ctxswitch wants to merge 1 commit into
cleanup/phase-1-bugsfrom
cleanup/phase-2-perf
Open

perf: remove hot-path allocation, syscalls, and redundant fsyncs#3
ctxswitch wants to merge 1 commit into
cleanup/phase-1-bugsfrom
cleanup/phase-2-perf

Conversation

@ctxswitch

Copy link
Copy Markdown
Owner

Summary

Second of six stacked PRs. Based on cleanup/phase-1-bugs (#2) — review that one
first. No structural change here; every item is strictly faster and same-or-smaller.

Widest blast radius: ChannelId::as_key heap-allocated a String on every metadata
key build, several times per request. It now encodes into a [u8; ULID_LEN] stack
buffer via Ulid::array_to_str — the function Display itself calls, so keys stay
byte-identical — and the .expect goes with it. artifact_key / reference_key /
eviction_key now size one Vec exactly instead of to_vec() +
extend_from_slice (two allocations plus a realloc each).

bind_reference takes a Durability; the proxy passes BestEffort, removing an
fsync from every upstream 304. remove_artifact drops its fsync to match evict,
which performs the same delete without one.

The remainder are independent: classify() runs once per request instead of twice,
the reqwest::Method::from_bytes round-trip and the ring .cloned() are gone,
prefetch-only header parsing moved inside the prefetch branch (both copies),
Identity-encoded files skip an fstat for a length known by construction, the
recent-use filter derives four probes from one hash by Kirsch-Mitzenmacher instead
of building a DefaultHasher per probe, the space ledger snapshots under a single
lock rather than three that could disagree, route_prefix is built only by the
three of fifteen handlers that read it, already-materialized metadata streams via
Body::from without holding a foreground permit, observe_http_request takes a
StatusCode, server_request_identity returns borrowed strings, channel_fence
returns the expiry from the record it already read, and Reference gained
is_valid/into_string so two call sites stop allocating to validate-and-discard.

Verification

make ci clean, including clippy --all-targets -- -D warnings.

  • On-disk format guards pass unchanged, confirming as_key is byte-identical:
    fresh_store_uses_only_channel_keys_and_record_schema_one and
    registered_channel_survives_reopen_without_legacy_policy_fields.

  • Crash-recovery coverage for the durability change passes unchanged:
    tests/integration/maintenance.rs and the channel deletion/resume tests.

  • New src/cache/recent_use_test.rs covers mark/rotate lifetime, identity keying,
    and a false-positive bound on the derived probes — confirmed to fail (25 false
    positives against a bound of 5) when the probes are forced to collapse onto one
    slot.

  • Log output for the single-classify change was checked against a temporary
    capturing subscriber and is unchanged on both data and control routes.

  • make ci

  • Documentation and examples are accurate

  • New or changed behavior has test coverage

Operational impact

Two deliberate behaviour changes:

  • GET on the Bazel action cache with an unparseable hash now returns 404 instead
    of 400
    , matching get_http_cache. The rejected key could only miss anyway. No
    test asserted the 400.
  • Reference binds from the proxy are no longer fsynced. A crash can now lose a
    reference→artifact binding that a 304 had refreshed; the next request re-validates
    upstream and rebinds. The artifact bodies these references point at were already
    published best-effort, so this aligns the metadata write with the data write
    rather than weakening a guarantee that held end-to-end.

No storage format, configuration, route, or security boundary changes. Metadata keys
are byte-identical, so no migration.

Review notes

Start with src/channel/identity.rs and the three key builders in
src/storage/metadata/rocksdb.rs — if those are right, the on-disk format is
untouched, which the two format guard tests independently confirm.

Two places where the implementation deviates from what was planned, both
deliberate:

  • delete_reference did not get a durability parameter. Its only caller is
    reached solely from DELETE /references/{r}, which is durable. The parameter
    would have exactly one call site, always passing Durable — dead flexibility that
    removes no fsync. bind_reference is where the win is and it is threaded.
  • server_request_identity returns (&str, Cow<'_, str>), not &'static str.
    The proxy arm's operation is a borrowed path segment and the fallback is
    MatchedPath::as_str(); neither is 'static. Tying both to the request lifetime
    is allocation-free all the same.

Noted for later phases, not addressed here: classify and server_request_identity
still each collect a Vec<&str> of path segments per request, needed for their
slice patterns — the largest remaining per-request allocation on those paths. After
the snapshot change, SpaceLedger::free_observed(), committed_since(), and
reserved() are referenced only by tests.

ChannelId::as_key heap-allocated a String on every metadata key build, several
times per request; it now encodes into a stack buffer via Ulid::array_to_str,
which is what Display already calls, so keys stay byte-identical. The three key
builders in rocksdb.rs size their Vec exactly instead of allocating twice and
reallocating.

bind_reference takes a Durability and the proxy passes BestEffort, removing an
fsync from every upstream 304; remove_artifact drops its fsync to match evict,
which performs the same delete.

The rest are independent and behaviour-preserving: classify runs once per
request rather than twice, the Method round-trip and the ring clone are gone,
prefetch-only header parsing moved inside the prefetch branch, Identity-encoded
files no longer fstat for a length known by construction, the recent-use filter
derives four probes from one hash, the space ledger snapshots under a single
lock, route_prefix is built only by the three handlers that read it, and
already-materialized metadata streams without holding a foreground permit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant