Develop T5b+T5c: the host-cache seal becomes a pure decision with sampled inputs - #1149
Merged
Conversation
…est pins it _seal_opencl_cache_policy() gathered nine facts about a pipeline node and decided, in four lines, whether that node must keep a host-RAM copy of its output. The decision moves to develop/pipe_cache_policy.h as a pure function of those nine named inputs; gathering them stays in dev_pixelpipe.c, which is the part that needs a pipe and a dev to look at. The reason is not tidiness. That flag is invisible to every check this project runs: it changes no exported pixel -- the export path drives the pipe directly (imageio_core.c) and never reaches the seal -- it changes no hash, and it logs nothing unless someone is already looking. The tranche plan requires preserving the per-piece outcomes, and nothing here could observe them. A pure function can be pinned by a test, so now it is. src/tests/unittests/test_pipe_cache_policy.c states the truth table: a CPU-only node raises the requirement for the node before it; a GPU node with no needs of its own raises nothing; each of the four own-output reasons raises this node's own flag and says nothing about upstream; each of the four own-input reasons raises upstream; and a NULL out-param is allowed, for the last node in the walk. The important one is _gpu_node_propagates_an_inherited_requirement, which is the bug that shipped: this was an `=' rather than an `||', so toggling on a GPU module with no host needs of its own erased the requirement a CPU-only module further downstream had correctly established, and that module then read the previous life of a rekeyed cacheline. I verified the test catches it rather than assuming so -- reintroducing the `=' fails that test and only that test. No behaviour change: same nine inputs, same boolean algebra, same order. Release, Debug (-Werror), nofeatures build; ctest 8/8; cycles 0, layering 187, module boundaries hold; export A/B 0 differing pixels, which as the header now says is not evidence about this flag either way.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…r predicate comes home _seal_opencl_cache_policy() read dev->gui_module and dev->color_picker.module LIVE, once per node, from the worker thread. A focus change or a picker toggle landing mid-walk therefore gave the early nodes one answer and the later nodes another, and the sealed pipe described no single moment. Both are now sampled once, before the walk. dt_iop_color_picker_force_cache() moves out of gui/color_picker_proxy.c and becomes _module_feeds_color_picker() next to its only caller. Nothing about it was GUI code -- it walks pipe nodes and compares module identities -- and the seal reaching down into gui/ for it is a call the T6 layer flip turns into a violation. Its doc comment claimed a second caller "when deciding whether a cache miss should trigger a preview recompute"; no such caller exists. Equivalent by construction, not by inspection alone: the added NULL early-out matches the old expression exactly, because dt_dev_pixelpipe_get_module_piece() returns NULL for a NULL module and dt_dev_pixelpipe_get_prev_enabled_piece() returns NULL for a NULL piece, so the old form already yielded FALSE for every non-NULL module when no picker was active. The other half of the tranche line -- having the GUI PUBLISH these two facts rather than the pipeline sample them -- is blocked on T6, and that is measured rather than assumed: tools/check_layering.sh fails the publisher wiring (+1, 187 -> 188) because gui/ is layer 4 and develop/ is layer 5, so today the pipeline may read the GUI but not the reverse. Noted in doc/develop-split.md against the T5 bullet. No pixel A/B accompanies this, and that is a statement rather than an omission: the export path drives the pipe directly and never reaches the seal, and headlessly both sampled fields are NULL, so an A/B here cannot execute the changed code at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aurelienpierre
changed the title
|
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.
The rest of the T5 tranche line ("
_seal_opencl_cache_policyreads pipe-owned flags"), in two commits._seal_opencl_cache_policy()walks a pipe backwards and decides, per node, whether that node must keep a host-RAM copy of its output.T5b — the decision becomes a pure function, and a test pins it
It gathered nine facts and decided in four lines. The decision moves to
develop/pipe_cache_policy.has a pure function of those nine named inputs; gathering them stays indev_pixelpipe.c, which is the part that needs a pipe and a dev to look at. Same inputs, same boolean algebra, same order.Why, which is not tidiness. The tranche plan requires preserving the per-piece
cache_output_on_ramoutcomes, and nothing this project runs can observe them: it changes no exported pixel (the export path drives the pipe directly,imageio_core.c, and never reaches the seal), changes no hash, and logs nothing unless someone is already looking. When it was last wrong, the symptom was a downstream module silently reading stale host bytes from a rekeyed cacheline, only with OpenCL enabled, found by dumping GPU buffers.A pure function can be pinned by a test. That is the whole argument for the shape.
src/tests/unittests/test_pipe_cache_policy.cstates the truth table. The important case is_gpu_node_propagates_an_inherited_requirement— the bug that shipped: an=rather than an||, so toggling on a GPU module with no host needs of its own (rawoverexposed) erased theTRUEa CPU-only module further downstream (dither) had correctly established;coloroutthen skipped its readback andditherread the cacheline's previous life, with every hash and ROI in the chain individually correct. I checked the test catches it rather than assuming: reintroducing the=fails that test and only that test.T5c — the seal samples the GUI's two facts once, and the picker predicate comes home
dev->gui_moduleanddev->color_picker.modulewere read live, once per node, from the worker thread. A focus change or picker toggle landing mid-walk gave the early nodes one answer and the later nodes another, so the sealed pipe described no single moment. Both are now sampled once, before the walk.dt_iop_color_picker_force_cache()leavesgui/color_picker_proxy.cand becomes_module_feeds_color_picker()beside its only caller. Nothing about it was GUI code — it walks pipe nodes and compares module identities — and the seal reaching down intogui/for it is a call the T6 flip turns into a violation. Its doc comment claimed a second caller "when deciding whether a cache miss should trigger a preview recompute"; no such caller exists.Equivalent by construction, not inspection: the added NULL early-out matches the old expression exactly, because
dt_dev_pixelpipe_get_module_piece()returns NULL for a NULL module anddt_dev_pixelpipe_get_prev_enabled_piece()returns NULL for a NULL piece — so the old form already yielded FALSE for every non-NULL module when no picker was active.What is deliberately NOT here, and why it is measured
Having the GUI publish those two facts — the T4b viewport shape — is the other half of the tranche line, and it is blocked on T6. I wrote it (a seqlock record, publishers at all 13 writers) and
tools/check_layering.shrejected it: +1, 187 → 188, becausegui/is layer 4 anddevelop/is layer 5, so today the pipeline may read the GUI but not the reverse. It becomes legal — and is then the natural first use of the flipped table — only after T6. Recorded against the T5 bullet indoc/develop-split.mdso the next pass does not rediscover it.Verification
Release, Debug (
-Werror), nofeatures build.ctest8/8.cycles 0,layering_violations 187(baseline), module boundaries hold.No pixel A/B, and that is a statement rather than an omission: the seal is not reached by the export path, and headlessly both sampled fields are NULL, so an A/B cannot execute the changed code at all. It would print 0 differing pixels and mean nothing — the same calibration T4b produced, where the harness printed 0 through all five real defects.
🤖 Generated with Claude Code