Doc: the src/control decomposition plan, measured - #1152
Merged
Conversation
src/control is 8168 lines over 19 files and carries 38 of the tree's 184 layering violations --
21% of the debt in 2% of the code. This is the survey and the sequence that removes it.
The mechanism, which is one line: control/control.h:56 includes libs/lib.h, because the progress
vtable types its callbacks in dt_lib_module_t -- a layer-7 type in a layer-3 struct. That puts 24
project headers into all 125 of control.h's includers, of which 72 name only dt_control_log().
Findings that are defects rather than layering, each cited in the file: a use-after-free on the
pipeline banner message (fixed separately, ahead of this plan -- a live crash should not wait for
a 13-PR sequence); progress cancellation destroying the mutex it holds; the signal bus dropping
four ownership-transferring signals before va_start, leaking on every CLI export; nine mutexes
declared, eight initialised, seven destroyed; and dt_control_log arming GSources on a main context
that never runs headless.
Six load-bearing claims were re-measured independently before this landed, because a census that
agrees with itself proves nothing. All six held, including the subtle one: control's 38-of-184
share is 41 of 187 before widgets/ moved to 2.5, the difference being exactly the three
control/ -> widgets/ edges the plan already accounts for.
It also kills a hypothesis of mine. I had proposed that the proxy.hinter TODO ("deleting it makes
g_free(dt_control_get_global()) segfault") was a stale-plugin artifact, on the grounds that
deleting the last member shrinks the allocation and a stale .so still writing that field would
write past the end. sizeof(dt_control_t) has exactly one consumer, darktable.c:897, in the
always-rebuilt main binary, and no offsets shift when the last member goes -- so that story
needs a .so older than the hinter refactor and explains nothing on its own. What survives is
pre-existing corruption whose landing site moved when the allocation shrank.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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.



You said
control/is many things at once and keeps biting during this refactor. This is the measurement of that, and the sequence that removes it. Doc only — no code.src/controlis 8168 lines over 19 files and carries 38 of the tree's 184 layering violations — 21% of the debt in 2% of the code.The mechanism is one line
control/control.h:56includeslibs/lib.h, because the progress vtable types its callbacks indt_lib_module_t— a layer-7 type in a layer-3 struct. That single edge puts 24 project headers into all 125 ofcontrol.h's includers. Of those 125, 72 name onlydt_control_log, and ten of the message-only files areiop/*.cplugin.sos compiling the view API for oneprintf.Removing it collapses
libs/lib.hfrom 141 compiling TUs to 37,control/jobs.hfrom 136 to 32,control/progress.hfrom 130 to 2.Defects found on the way, not layering
Each is cited in the file: progress cancellation destroying the mutex it holds (user-triggerable on a queued job); the signal bus returning before
va_start, so four ownership-transferring signals leak on every drop — oneGListnode perdt_image_cache_write_release()in every CLI export; nine mutexes declared, eight initialised, seven destroyed (global_mutexused and never destroyed,image_mutexdeclared and nothing else); anddt_control_logarmingg_timeout_add/g_idle_addon a main context that never runs headless, i.e. unbounded GSource accumulation in every batch export.The use-after-free on the pipeline banner message is already fixed, separately and ahead of this plan (#1151) — a live crash had no business waiting for a 13-PR sequence.
On trusting this
The plan came out of a multi-agent census, so I re-measured six load-bearing claims independently before landing it, because a census that agrees with itself proves nothing. All six held — including the subtle one: control's 38-of-184 share is 41 of 187 before
widgets/moved to 2.5, the difference being exactly the threecontrol/ → widgets/edges the plan already accounts for. And('control', 1)really does give 165.It also kills a hypothesis of mine. I had suggested the
proxy.hinterTODO ("deleting it makesg_free(dt_control_get_global())segfault") was a stale-plugin artifact.sizeof(dt_control_t)has exactly one consumer,darktable.c:897, in the always-rebuilt main binary, and deleting the last member shifts no offsets — so that story needs a.soolder than the hinter refactor and explains nothing on its own. What survives is pre-existing corruption whose landing site moved when the allocation shrank. The plan deletes the field in PR10, with the allocator pairing fixed, rather than as a standalone experiment — which is what I had been about to propose, and would have been wrong.Shape of the answer
src/controlsurvives, redefined as work that has not happened yet — the scheduler, the progress objects describing it, the signal bus announcing it, the flag saying whether the loop is alive. What leaves is the GTK: the input router, the cursor, the view-switch shims, log/toast rendering,crawler.c's 580-line GtkTreeView.13 PRs, each landable and gate-passing alone. Every header split lands in place, at the same layer, before any file moves, so the wide include-repoint PRs are ratchet-neutral by construction and nothing is renamed — a tree-wide
dt_control_logrename would detonate the in-flightt4b…t6astack across its 311 call sites in 84 files. The layer move to 1 is the closing one-liner, deliberately last: banking the −19 early stops the gate measuring the debt it exists to measure.Open questions are listed at the end with a recommended answer each, so they're yours to overrule rather than mine to assume.
🤖 Generated with Claude Code