Re-fix stupid "fix" for 663 - #745
Merged
Merged
Conversation
The suite pins its measurements to worker 1 / thread 1 so the counts do not depend on the worker topology the testsuite happens to run under, but only the `measure_steady_state_allocs` call ran under that scope -- the `let`-block DArrays each test operates on were built outside it. That gap is invisible today only because the scheduler currently piles every task onto worker 1 (#663). `allocate_array` tasks take no `Chunk` inputs, so they carry zero data-transfer cost and nothing anchors them to a worker; as soon as the scheduler can see per-processor load it spreads them across the cluster -- correctly, that is what a distributed array is -- and the pinned measurement then pays to pull those chunks back. The result is a 5x "allocation regression" that is really cross-worker data movement, i.e. exactly the topology dependence the pinning exists to prevent. Wrap the whole testset body, so the comment below it ("Everything above ran inside `Dagger.with_options(scope=ALLOC_TEST_SCOPE)`") is true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8091734 restored the est_business term dropped in b16afad, but summed every candidate processor's pressure into a single scalar and folded that into `est_time_util`. `estimate_task_costs!` compares candidates, so a term identical across all of them is a constant offset: it cancels out of both the comparison and the `sort!`, leaving the ordering bit-for-bit what it was before. Worker 1 still always won by exactly `task_xfer_cost` and #663 was untouched. Look the pressure up per candidate instead, as the pre-b16afad8 code did, so a busy processor really is costlier than an idle one. Gathered in its own pass over `procs` into a `@reusable_vector` indexed alongside them: `worker_time_pressure` is locked once rather than per processor, no lock-order edge to `worker_transfer_rate` is created, and no `Processor` hashing is added beyond the two lookups the pressure read inherently needs. Steady-state allocations are unchanged (matmul! 27,651 vs 27,842 baseline; full test/allocations.jl green). 40 sleeping tasks over 4 workers under the default scope: before: [1 => 40] after: [1 => 10, 2 => 10, 3 => 10, 4 => 10] The existing cost assertions could not catch this: they run against an idle scheduler where every pressure is zero, which makes the summed and per-processor forms indistinguishable. Add a testset that puts pressure on one candidate and asserts the other wins despite its transfer cost. Fixes #663 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`do_task` reports transfer_rate as `transfer_size / (transfer_time/10^9)` with transfer_time in nanoseconds, i.e. in bytes per *second*. The cost model then divides a byte count by it, producing a duration in seconds, and adds it to `est_time_util` and `task_xfer_cost`, which are both in nanoseconds. So the data-transfer term was being discounted by a factor of 10^9: with the default 1 MB/s rate, moving an 8 KiB chunk cross-worker scored 0.008 instead of 8.2e6, i.e. no measurable preference for keeping a task near its data. Scale the quotient to nanoseconds. Split out from the per-processor pressure fix, which it does not affect either way (the multi-candidate decisions in test/allocations.jl are all `allocate_array` tasks, whose zero-Chunk inputs give them no transfer cost to weigh) -- but leaving the mismatch in place means locality has essentially no say in placement now that pressure does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Dagger benchmarks:
|
| master | dirty | master / dirty | |
|---|---|---|---|
| array/dagger/N=1024 (block 512)/add (X + X) | 5.24 ± 1.4 ms | 2.99 ± 1.4 ms | 1.75 ± 0.95 |
| array/dagger/N=1024 (block 512)/alloc (rand) | 1.75 ± 0.22 ms | 1.98 ± 0.6 ms | 0.884 ± 0.29 |
| array/dagger/N=1024 (block 512)/broadcast (X .+ 1) | 2.13 ± 1.2 ms | 1.84 ± 1.3 ms | 1.16 ± 1 |
| array/dagger/N=1024 (block 512)/map (sin.(X)) | 5.85 ± 0.81 ms | 5.38 ± 0.85 ms | 1.09 ± 0.23 |
| array/dagger/N=1024 (block 512)/norm | 1.16 ± 0.66 ms | 2.03 ± 0.83 ms | 0.572 ± 0.4 |
| array/dagger/N=1024 (block 512)/reduce (sum) | 5.08 ± 0.8 ms | 4.59 ± 1.8 ms | 1.11 ± 0.47 |
| array/dagger/N=1024 (block 512)/transpose (permutedims) | 4.41 ± 0.61 ms | 3.97 ± 0.47 ms | 1.11 ± 0.2 |
| array/dagger/N=256 (block 256)/add (X + X) | 0.917 ± 0.12 ms | 0.779 ± 0.52 ms | 1.18 ± 0.8 |
| array/dagger/N=256 (block 256)/alloc (rand) | 0.638 ± 0.0057 ms | 1.52 ± 0.14 ms | 0.419 ± 0.04 |
| array/dagger/N=256 (block 256)/broadcast (X .+ 1) | 1.29 ± 0.81 ms | 0.561 ± 0.17 ms | 2.3 ± 1.6 |
| array/dagger/N=256 (block 256)/map (sin.(X)) | 0.959 ± 0.27 ms | 0.878 ± 0.041 ms | 1.09 ± 0.31 |
| array/dagger/N=256 (block 256)/norm | 0.488 ± 0.075 ms | 0.433 ± 0.075 ms | 1.13 ± 0.26 |
| array/dagger/N=256 (block 256)/reduce (sum) | 0.89 ± 0.031 ms | 0.815 ± 0.15 ms | 1.09 ± 0.2 |
| array/dagger/N=256 (block 256)/transpose (permutedims) | 0.781 ± 0.13 ms | 0.724 ± 0.0098 ms | 1.08 ± 0.18 |
| linalg/dagger/N=1024 (block 512)/cholesky | 19.3 ± 4.9 ms | 20.8 ± 1.7 ms | 0.929 ± 0.25 |
| linalg/dagger/N=1024 (block 512)/lu | 0.0413 ± 0.0014 s | 0.0415 ± 0.0025 s | 0.995 ± 0.069 |
| linalg/dagger/N=1024 (block 512)/matmul (A*A) | 0.0431 ± 0.0051 s | 0.0493 ± 0.0044 s | 0.875 ± 0.13 |
| linalg/dagger/N=1024 (block 512)/matvec (A*x) | 3.07 ± 1 ms | 5.9 ± 3.9 ms | 0.52 ± 0.38 |
| linalg/dagger/N=1024 (block 512)/qr | 0.116 ± 0.002 s | 0.116 ± 0.0065 s | 0.997 ± 0.059 |
| linalg/dagger/N=1024 (block 512)/solve (A\b via lu) | 0.0582 ± 0.0095 s | 0.0556 ± 0.00088 s | 1.05 ± 0.17 |
| linalg/dagger/N=1024 (block 512)/svd | 0.036 h | 0.0366 h | 0.983 |
| linalg/dagger/N=1024 (block 512)/syrk (A'*A) | 0.0318 ± 0.0047 s | 0.0366 ± 0.0046 s | 0.869 ± 0.17 |
| linalg/dagger/N=256 (block 256)/cholesky | 5.76 ± 4.7 ms | 2.25 ± 0.063 ms | 2.55 ± 2.1 |
| linalg/dagger/N=256 (block 256)/lu | 4.31 ± 0.71 ms | 5.1 ± 1.4 ms | 0.845 ± 0.27 |
| linalg/dagger/N=256 (block 256)/matmul (A*A) | 2.65 ± 0.72 ms | 2.22 ± 0.37 ms | 1.19 ± 0.38 |
| linalg/dagger/N=256 (block 256)/matvec (A*x) | 0.952 ± 0.03 ms | 1.06 ± 1.3 ms | 0.894 ± 1.1 |
| linalg/dagger/N=256 (block 256)/qr | 7.01 ± 1.4 ms | 5.49 ± 0.69 ms | 1.28 ± 0.3 |
| linalg/dagger/N=256 (block 256)/solve (A\b via lu) | 11.9 ± 5.3 ms | 14.1 ± 5.4 ms | 0.846 ± 0.5 |
| linalg/dagger/N=256 (block 256)/svd | 0.664 ± 0.0077 s | 0.536 ± 0.0059 s | 1.24 ± 0.02 |
| linalg/dagger/N=256 (block 256)/syrk (A'*A) | 3.11 ± 4.2 ms | 2.66 ± 1.3 ms | 1.17 ± 1.7 |
| stencil/dagger/N=1024 (block 512)/alloc (neighbors Wrap) | 7.83 ± 2.5 ms | 6.25 ± 0.86 ms | 1.25 ± 0.44 |
| stencil/dagger/N=1024 (block 512)/assign (const) | 0.92 ± 0.48 ms | 1.21 ± 0.35 ms | 0.758 ± 0.45 |
| stencil/dagger/N=1024 (block 512)/multi-expr | 2.54 ± 2.8 ms | 2.07 ± 0.067 ms | 1.23 ± 1.3 |
| stencil/dagger/N=1024 (block 512)/neighbors (Clamp) | 7.95 ± 2.4 ms | 6.48 ± 0.48 ms | 1.23 ± 0.38 |
| stencil/dagger/N=1024 (block 512)/neighbors (Pad) | 6.86 ± 1.7 ms | 6.77 ± 1.2 ms | 1.01 ± 0.31 |
| stencil/dagger/N=1024 (block 512)/neighbors (Reflect) | 6.79 ± 2 ms | 8.41 ± 2.6 ms | 0.807 ± 0.34 |
| stencil/dagger/N=1024 (block 512)/neighbors (Wrap) | 6.41 ± 0.81 ms | 6.23 ± 0.13 ms | 1.03 ± 0.13 |
| stencil/dagger/N=1024 (block 512)/update (+) | 1.79 ± 0.57 ms | 1.57 ± 2.6 ms | 1.14 ± 1.9 |
| stencil/dagger/N=256 (block 256)/alloc (neighbors Wrap) | 1.79 ± 0.048 ms | 2 ± 0.53 ms | 0.897 ± 0.24 |
| stencil/dagger/N=256 (block 256)/assign (const) | 0.524 ± 0.029 ms | 0.538 ± 0.045 ms | 0.975 ± 0.097 |
| stencil/dagger/N=256 (block 256)/multi-expr | 2.03 ± 1.5 ms | 3.01 ± 1.6 ms | 0.674 ± 0.6 |
| stencil/dagger/N=256 (block 256)/neighbors (Clamp) | 1.61 ± 0.075 ms | 1.77 ± 0.13 ms | 0.911 ± 0.079 |
| stencil/dagger/N=256 (block 256)/neighbors (Pad) | 1.82 ± 0.92 ms | 1.83 ± 0.0084 ms | 0.996 ± 0.5 |
| stencil/dagger/N=256 (block 256)/neighbors (Reflect) | 1.59 ± 0.43 ms | 1.71 ± 0.16 ms | 0.929 ± 0.27 |
| stencil/dagger/N=256 (block 256)/neighbors (Wrap) | 1.25 ± 0.11 ms | 1.54 ± 0.19 ms | 0.811 ± 0.12 |
| stencil/dagger/N=256 (block 256)/update (+) | 0.87 ± 0.4 ms | 0.868 ± 2.2 ms | 1 ± 2.6 |
| time_to_load | 1.1 ± 0.018 s | 1.1 ± 0.016 s | 0.999 ± 0.022 |
Allocations / memory
| master | dirty | master / dirty | |
|---|---|---|---|
| array/dagger/N=1024 (block 512)/add (X + X) | 5.41 k allocs: 8.21 MB | 5.42 k allocs: 8.21 MB | 1 |
| array/dagger/N=1024 (block 512)/alloc (rand) | 2.71 k allocs: 8.09 MB | 2.69 k allocs: 8.09 MB | 1 |
| array/dagger/N=1024 (block 512)/broadcast (X .+ 1) | 2.38 k allocs: 8.08 MB | 2.37 k allocs: 8.08 MB | 1 |
| array/dagger/N=1024 (block 512)/map (sin.(X)) | 2.06 k allocs: 8.07 MB | 2.08 k allocs: 8.07 MB | 1 |
| array/dagger/N=1024 (block 512)/norm | 2.62 k allocs: 0.0873 MB | 2.61 k allocs: 0.0878 MB | 0.995 |
| array/dagger/N=1024 (block 512)/reduce (sum) | 4.32 k allocs: 0.148 MB | 4.64 k allocs: 0.162 MB | 0.914 |
| array/dagger/N=1024 (block 512)/transpose (permutedims) | 3.26 k allocs: 8.13 MB | 3.28 k allocs: 8.13 MB | 1 |
| array/dagger/N=256 (block 256)/add (X + X) | 1.71 k allocs: 0.574 MB | 1.7 k allocs: 0.574 MB | 1 |
| array/dagger/N=256 (block 256)/alloc (rand) | 0.752 k allocs: 0.526 MB | 0.742 k allocs: 0.526 MB | 1 |
| array/dagger/N=256 (block 256)/broadcast (X .+ 1) | 0.643 k allocs: 0.522 MB | 0.638 k allocs: 0.522 MB | 1 |
| array/dagger/N=256 (block 256)/map (sin.(X)) | 0.558 k allocs: 0.519 MB | 0.561 k allocs: 0.519 MB | 1 |
| array/dagger/N=256 (block 256)/norm | 0.71 k allocs: 24.5 kB | 0.702 k allocs: 24.4 kB | 1 |
| array/dagger/N=256 (block 256)/reduce (sum) | 1.29 k allocs: 0.0464 MB | 1.06 k allocs: 0.0383 MB | 1.21 |
| array/dagger/N=256 (block 256)/transpose (permutedims) | 1.1 k allocs: 0.551 MB | 1.09 k allocs: 0.551 MB | 1 |
| linalg/dagger/N=1024 (block 512)/cholesky | 7.77 k allocs: 10.3 MB | 7.74 k allocs: 10.3 MB | 1 |
| linalg/dagger/N=1024 (block 512)/lu | 15.6 k allocs: 14.6 MB | 15.4 k allocs: 14.6 MB | 1 |
| linalg/dagger/N=1024 (block 512)/matmul (A*A) | 7.66 k allocs: 8.28 MB | 7.6 k allocs: 8.28 MB | 1 |
| linalg/dagger/N=1024 (block 512)/matvec (A*x) | 6.2 k allocs: 0.247 MB | 6.07 k allocs: 0.244 MB | 1.01 |
| linalg/dagger/N=1024 (block 512)/qr | 11.2 k allocs: 9.58 MB | 11.4 k allocs: 9.59 MB | 0.999 |
| linalg/dagger/N=1024 (block 512)/solve (A\b via lu) | 29.5 k allocs: 15.2 MB | 29.6 k allocs: 15.2 MB | 1 |
| linalg/dagger/N=1024 (block 512)/svd | 0.238 M allocs: 0.205 GB | 0.0481 M allocs: 0.197 GB | 1.04 |
| linalg/dagger/N=1024 (block 512)/syrk (A'*A) | 8.86 k allocs: 24.4 MB | 8.78 k allocs: 24.4 MB | 1 |
| linalg/dagger/N=256 (block 256)/cholesky | 3.03 k allocs: 0.633 MB | 2.98 k allocs: 0.632 MB | 1 |
| linalg/dagger/N=256 (block 256)/lu | 5.38 k allocs: 1.24 MB | 5.34 k allocs: 1.24 MB | 1 |
| linalg/dagger/N=256 (block 256)/matmul (A*A) | 1.93 k allocs: 0.582 MB | 1.9 k allocs: 0.581 MB | 1 |
| linalg/dagger/N=256 (block 256)/matvec (A*x) | 2.43 k allocs: 0.104 MB | 2.41 k allocs: 0.103 MB | 1 |
| linalg/dagger/N=256 (block 256)/qr | 3.46 k allocs: 0.779 MB | 3.4 k allocs: 0.778 MB | 1 |
| linalg/dagger/N=256 (block 256)/solve (A\b via lu) | 11.9 k allocs: 1.53 MB | 11.8 k allocs: 1.53 MB | 1 |
| linalg/dagger/N=256 (block 256)/svd | 15.4 k allocs: 6.71 MB | 15.3 k allocs: 6.71 MB | 1 |
| linalg/dagger/N=256 (block 256)/syrk (A'*A) | 2.96 k allocs: 2.63 MB | 2.94 k allocs: 2.63 MB | 1 |
| stencil/dagger/N=1024 (block 512)/alloc (neighbors Wrap) | 4.61 k allocs: 8.2 MB | 4.65 k allocs: 8.2 MB | 1 |
| stencil/dagger/N=1024 (block 512)/assign (const) | 2.07 k allocs: 0.0915 MB | 2.14 k allocs: 0.0942 MB | 0.972 |
| stencil/dagger/N=1024 (block 512)/multi-expr | 4.68 k allocs: 0.208 MB | 4.74 k allocs: 0.211 MB | 0.987 |
| stencil/dagger/N=1024 (block 512)/neighbors (Clamp) | 3.69 k allocs: 0.229 MB | 3.71 k allocs: 0.231 MB | 0.995 |
| stencil/dagger/N=1024 (block 512)/neighbors (Pad) | 3.63 k allocs: 0.228 MB | 3.58 k allocs: 0.226 MB | 1.01 |
| stencil/dagger/N=1024 (block 512)/neighbors (Reflect) | 3.7 k allocs: 0.261 MB | 3.71 k allocs: 0.262 MB | 0.997 |
| stencil/dagger/N=1024 (block 512)/neighbors (Wrap) | 3.54 k allocs: 0.166 MB | 3.52 k allocs: 0.165 MB | 1 |
| stencil/dagger/N=1024 (block 512)/update (+) | 2.53 k allocs: 0.113 MB | 2.52 k allocs: 0.113 MB | 0.999 |
| stencil/dagger/N=256 (block 256)/alloc (neighbors Wrap) | 1.53 k allocs: 0.574 MB | 1.52 k allocs: 0.573 MB | 1 |
| stencil/dagger/N=256 (block 256)/assign (const) | 0.806 k allocs: 0.0406 MB | 0.791 k allocs: 0.0403 MB | 1.01 |
| stencil/dagger/N=256 (block 256)/multi-expr | 1.73 k allocs: 0.0866 MB | 1.69 k allocs: 0.0856 MB | 1.01 |
| stencil/dagger/N=256 (block 256)/neighbors (Clamp) | 1.28 k allocs: 0.0726 MB | 1.23 k allocs: 0.0711 MB | 1.02 |
| stencil/dagger/N=256 (block 256)/neighbors (Pad) | 1.19 k allocs: 0.0695 MB | 1.18 k allocs: 0.0693 MB | 1 |
| stencil/dagger/N=256 (block 256)/neighbors (Reflect) | 1.24 k allocs: 0.0787 MB | 1.23 k allocs: 0.0785 MB | 1 |
| stencil/dagger/N=256 (block 256)/neighbors (Wrap) | 1.24 k allocs: 0.0645 MB | 1.17 k allocs: 0.0618 MB | 1.04 |
| stencil/dagger/N=256 (block 256)/update (+) | 0.92 k allocs: 0.046 MB | 0.909 k allocs: 0.0458 MB | 1 |
| time_to_load | 0.147 k allocs: 10.8 kB | 0.147 k allocs: 10.8 kB | 1 |
Plots
⚠️ Regressions (time > 25.0% and outside the reported ±spread; allocs/memory > 25.0%)
array/dagger/N=256 (block 256)/alloc (rand)(time): +138.4%
Improvements
linalg/dagger/N=1024 (block 512)/svd(allocs): -79.8%
Within noise (7 metric(s) past threshold but inside the ±spread; not counted)
linalg/dagger/N=1024 (block 512)/matvec (A*x)(time): 92.3%array/dagger/N=1024 (block 512)/norm(time): 74.9%stencil/dagger/N=256 (block 256)/multi-expr(time): 48.3%stencil/dagger/N=1024 (block 512)/assign (const)(time): 31.9%array/dagger/N=1024 (block 512)/add (X + X)(time): -42.9%array/dagger/N=256 (block 256)/broadcast (X .+ 1)(time): -56.5%linalg/dagger/N=256 (block 256)/cholesky(time): -60.8%
Full results and plots (download the benchmark-results artifact).
`multi_span_copy!` picked the processor whose backend lock guards its
kernel launch with `task_processor()`, which asserts `DTASK_TLS[]` is a
`DTaskTLS`. That holds inside a DTask, but this is also reached from
outside one: `move!(::RemainderAliasing, ...)` hands the source-side
gather to the owning worker with `remotecall_fetch`, and that closure runs
in a Distributed message-handler task, which carries no Dagger TLS at all.
So a cross-worker remainder move whose source is a device array died with
TypeError: in typeassert, expected Dagger.DTaskTLS, got a value of type Nothing
[1] get_tls @ src/task-tls.jl:29
[2] task_processor @ src/task-tls.jl:81
[3] multi_span_copy! @ src/utils/span_copy.jl:132
[4] _multi_span_gather_packed! @ src/utils/span_copy.jl:184
[5] multi_span_gather! @ src/utils/span_copy.jl:260
[6] #420 @ src/datadeps/remainders.jl:516
The same-worker cases were fine and hid this: `remotecall_fetch` to
`myid()` runs the closure inline on the calling task, so the DTask's TLS
was still in scope. Only a genuinely remote source reaches the
message-handler task, and only a device source reaches the kernel path
(a host `Array` source returns from `multi_span_copy!`'s memcpy fast path
before the launch).
Fall back to a processor of the value's own memory space when there is no
DTask TLS -- that is the processor owning the lock we want anyway, and
`memory_space`/`processors` are both TLS-free for device arrays. Inside a
DTask `task_processor()` still wins, which keeps the MPI path working
(`gpu_kernel_lock(::MPIProcessor)` unwraps the wrapper that only the TLS
carries).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jpsamaroo
force-pushed
the
jps/fix-stupid-663
branch
from
August 27, 2026 19:00
e015a80 to
920b2f6
Compare
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.
#744 did a dumb and summed the pressure across all workers as one scalar, rather than segregating them by worker, leading to #663 not actually being fixed - this does the right thing and corrects the issue, and also corrects a few other bugs found along the way.
Written by Claude Opus