Make TimespanLogging cheap enough to leave on - #751
Merged
Merged
Conversation
Record into per-thread typed chunk lists and run consumers at collect time, so Datadeps logging-on is ~1.2× instead of ~8×. Categories are Log* types (generated as_old_id, lazy IDs so Dagger precompile does not mutate TimespanLogging), with per-category max_chunks via Preferences. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Dagger benchmarks:
|
…ooled Set
LogAddThunk's logged payload held a live reference to the task's `Options`
struct, whose `syncdeps` field datadeps' queue.jl nulls out and returns to a
pool (`return_syncdeps_set!`) immediately after a synchronous submission
consumes it. That was harmless under the old eager-consumer design, where
`TaskDependencies` read `syncdeps` synchronously at the `timespan_finish`
call site -- before the pool reclaimed it. Under the new deferred-consumer
design ("Make TimespanLogging cheap enough to leave on", e8b7f47), the
event is only projected into the legacy `Event` shape and consumed at
`fetch_logs!()` time, by which point the pooled `Set` this event still
references has already been nulled and possibly handed to a later task.
`ev.timeline.options.syncdeps` then reads back `nothing`, so
`TaskDependencies` reports every task as having zero dependencies.
This silently broke `test/datadeps.jl`'s dependency-graph reconstruction
(`build_dataflow`/`test_dataflow`), which only ever consults task IDs, not
data content, so nothing crashed -- flows just looked undominated. 146 of
1464 datadeps tests failed as a result; a minimal repro (`In(A)` then
`Out(A)`, two tasks) reproduces it standalone and confirms the fix restores
the same `taskdeps` result as the pre-e8b7f47f baseline.
`TaskDependencies` is the only reader of `LogAddThunk`'s `options.syncdeps`
anywhere in the tree, so the fix only needs to keep that one field alive:
snapshot it into a bare `(;syncdeps=...)` NamedTuple at log time, replacing
the live `Options` reference the logged payload used to carry.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GaPaEAUCJMwpQjFFPbU7Ck
The main Buildkite/GHA test matrix already devs lib/TimespanLogging into the root project, but the OpenCL, OpenCL+MPI, CUDA/ROCm/oneAPI/ Metal (plain and +MPI), and GHA mpi-cpu/mpi-opencl jobs instantiate a separate test/*env project and only `Pkg.develop` the Dagger checkout into it -- TimespanLogging stayed pinned to whatever's in the General registry there, since it's only a transitive dependency and isn't covered by Dagger's own [sources]. Add the same `Pkg.develop(path="lib/TimespanLogging")` call to each of those jobs so they all exercise the fast local TimespanLogging. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GaPaEAUCJMwpQjFFPbU7Ck
The "Manual" MultiEventLog test drained the shared per-thread event buffers once, after a fixed sleep(1). get_logs! is destructive, and compute(ctx, c) returning doesn't guarantee all scheduler teardown activity has finished landing its events -- on a loaded/oversubscribed runner (e.g. CI's shared vCPUs), that trailing activity can still be in flight past the 1-second mark, dropping events the test then expects to see. Poll and merge across drains instead, requiring a short quiet period (no newly-drained events) after the expected categories show up, bounded by an overall deadline. Verified the naive version of this fix (break as soon as the expected categories appear, without the quiet-period check) actively reproduces a related failure -- trailing events land on the *next* drain instead, which the test's own "should be empty after we're done" check then catches. That's direct evidence of real trailing async activity after compute() returns, and the likely root cause of the "Julia 1 - ubuntu-latest" flake on PR #751 (not locally reproducible even after 10 repeated runs, including one with the CPU count pinned to match the runner). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GaPaEAUCJMwpQjFFPbU7Ck
Two sequential Pkg.develop calls (Dagger, then TimespanLogging) into
the same test/*env project broke dependency resolution on Julia 1.10:
the second call's targeted resolve no longer saw Dagger as pinned to
its dev path and instead treated it as fixed to the registered 0.22.4,
which only allows TimespanLogging 0.1.x -- unsatisfiable against
Dagger's own TimespanLogging = "0.2" compat bound.
Reproduced directly with a from-scratch depot on Julia 1.10:
ERROR: Unsatisfiable requirements detected for package
TimespanLogging [a526e669]: ... restricted to versions 0.2 by
Dagger [d58978e5] -- no versions left
Batching both packages into one Pkg.develop([...]) call resolves them
together against a single dev-pinned Dagger and avoids the conflict;
confirmed against the same from-scratch 1.10 depot.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GaPaEAUCJMwpQjFFPbU7Ck
Two more resolver bugs in the same family as 33eff72, both on a from-scratch checkout with no cached Manifest.toml: - "Julia 1 - TimespanLogging" called Pkg.instantiate() *before* Pkg.develop(path="lib/TimespanLogging"). Instantiate resolves the root project's declared TimespanLogging = "0.2" compat straight against the registry, which only has 0.1.0-0.1.1 published -- unsatisfiable, since the local 0.2.0 doesn't exist as a registered release yet. Fix: develop before instantiate, matching every other job's order. - "Julia 1 - DaggerWebDash" did three sequential Pkg.develop calls (Dagger, then TimespanLogging, then DaggerWebDash). Confirmed this hits the same class of bug as 33eff72 -- and NOT just on Julia 1.10: reproduced on 1.12 too with a from-scratch depot. Batching into one Pkg.develop([...]) call fixes the sequencing issue, but surfaces a real, separate conflict once TimespanLogging correctly resolves to its dev-pinned 0.2.0: DaggerWebDash's own compat pins `TimespanLogging = "0.1"`, which excludes it. DaggerWebDash's actual usage (MultiEventLog, LogWindow, Events.creation_hook/deletion_hook, init_similar) is all still present in TimespanLogging 0.2, so this is a compat-bound update, not an API fix: widen to "0.1, 0.2". Verified both against a from-scratch JULIA_DEPOT_PATH: the TimespanLogging job's fixed command runs its full suite (72/72), and DaggerWebDash's fixed command resolves, precompiles, and builds DaggerWebDash and its extensions cleanly (the one remaining test failure is an unrelated, pre-existing race between the D3Renderer's HTTP server startup and an immediate curl probe with no wait/retry -- last touched in 738ea29, well before this branch). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GaPaEAUCJMwpQjFFPbU7Ck
The pre-rewrite version had a docstring directly on the
LocalEventLog-specific get_logs! method; during the collect-time
rewrite it got repurposed into a docstring on the LocalEventLog type
itself, leaving the get_logs! generic function with no docs at all.
Broke the Documenter build:
Error: no docs found for 'get_logs!' in `@docs` block in
docs/src/api-timespanlogging/functions.md:11-15
Add a docstring to the general ctx/sink-dispatching entry point,
describing the current per-sink projection behavior instead of the
old single-sink implementation. Verified docs/make.jl builds clean
(no `no docs found` / no docs_block error).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GaPaEAUCJMwpQjFFPbU7Ck
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.
Makes TimespanLogging a ton faster and allocate less.
Written by Cursor Grok