compute, timely-util: tech debt cleanup#36476
Draft
antiguru wants to merge 2 commits into
Draft
Conversation
Bundle of mechanical cleanups identified during a tech-debt audit of
src/compute and src/timely-util (Linear: CLU-81).
* timely-util/columnation: add SAFETY comments to all unsafe blocks in
ColumnationStack (copy, clear, copy_destructured), per project rule.
The invariant: Region::copy returns aliasing items; local Vec<T> is a
tombstone whose Drop glue is suppressed via set_len(0) before drop.
* timely-util/reclock: document the pusher contract. The function
returns a Box<dyn Push<...>> that callers must drive, otherwise the
operator silently deadlocks.
* timely-util/scope_label: re-implement the timely-scope profiling label
that was gutted in the timely v4 upgrade. Wraps the body of a
dataflow in a same-timestamp subgraph installed as a LabelledOperator;
each schedule() call runs inside a custom_labels scope tagged
timely-scope=<name>. The v4 in-place Scope wrapper is no longer
expressible (Scope is now a concrete struct), so the API changes from
let scope = scope.with_label(); to scope.with_label(|scope| { ... }).
All seven call sites in compute and storage are updated.
* compute/arrangement: add debug_assert! for the PaddedTrace invariant
(padded_since < trace logical compaction) at both mutation sites in
into_padded and set_logical_compaction.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`LabelledOperator` is a private struct; the module-level doc comment linked to it as if public. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Motivation
Bundle of mechanical cleanups identified during a tech-debt audit of
src/computeandsrc/timely-util(CLU-81). One PR to avoid spam; each item is independent and can be reviewed in isolation by file.Description
timely-util/columnation: add
SAFETYcomments on everyunsafeblock inColumnationStack(copy,clear, twocopy_destructured). Project rule: everyunsafeneeds aSAFETYjustification. Invariant:Region::copyreturns aliasing items;local: Vec<T>is a tombstone whoseDropglue is suppressed viaset_len(0)before drop.timely-util/reclock: document the pusher contract under a
# Pusher contractheading. The function returns aBox<dyn Push<...>>that callers must drive — if it is dropped without ever being used, the operator silently deadlocks at the consumer.timely-util/scope_label: re-implement the
timely-scopeprofiling label that was gutted in the timely v4 upgrade (was a no-opwith_label(self) -> Self). New design wraps the body of a dataflow in a same-timestamp subgraph installed as aLabelledOperator; eachschedule()call runs inside acustom_labelsscope taggedtimely-scope=<name>. The pre-v4 in-placeScopewrapper is no longer expressible (timely v4 madeScopea concrete struct), so the API changes fromlet scope = scope.with_label();toscope.with_label(|scope| { ... }). All seven call sites in compute and storage are migrated. There is one extra subgraph layer per dataflow now, with associated progress-tracking overhead.compute/arrangement: add
debug_assert!for thePaddedTraceinvariant (padded_sincestrictly less than the inner trace's logical compaction frontier) at both mutation sites ininto_paddedandset_logical_compaction. The invariant was previously comment-only and unenforced.The large line counts in the storage/compute
render.rsfiles are mostlycargo fmtre-indentation from wrapping the dataflow body in a closure; the actual semantic change at each site is one extra level of nesting.Verification
cargo check --workspacepasses.cargo clippy --workspace --all-targets -- -D warningspasses.bin/lintrustfmt step passes (other unrelated lint failures: missingbuf,trufflehog,protobuftooling in this environment).