Don't invalidate the pixelpipe cache on every commit when a raster mask is used

[masterpiga]

I stumbled onto this while profiling some mask changes. There, a raster-masked pipe re-executed from the raster source on every mask edit (~2.8 s), instead of recomputing only the edited last module (AgX, where the raster mask was being used, ~0.17 s). The wall-clock cost will scale with image size and how early the raster source sits.

dt_iop_commit_blend_params() computes whether a raster-mask user registration is actually new:

const gboolean new = g_hash_table_insert(candidate->raster_mask.source.users, module,
                                         GINT_TO_POINTER(blendop_params->raster_mask_id));

…uses new only in a debug print, and then returns candidate unconditionally.

The caller treats any non-NULL return as "a source just gained a raster user" and invalidates the pipe cache downstream of that source:

if(blendop_params->mask_mode & DEVELOP_MASK_RASTER && new_raster)
  dt_dev_pixelpipe_cache_invalidate_later(pipe, new_raster->iop_order, "blend new raster: ");

Since the return is non-NULL on every commit — not just the first — any edit anywhere invalidates every cacheline at or after the raster source's iop_order.

Why this matters

The invalidation exists for a narrow, real case: a source that gains a new user doesn't change its own params, so its cacheline hash is unchanged and it could be served from cache without ever writing the raster mask the new user depends on. That's correct. But it should fire once, when the user is genuinely added.

As written, it fires on every history commit. If the raster source sits early in the pipe (a common setup — mask painted on an early exposure/tone module, consumed by a later one), every slider move discards the cached output of the source and everything after it, forcing a full re-render of the pipe tail. The user is editing one late module, but pays for re-computing modules they never touched.

This is invisible on small images and short pipes, and increasingly punishing as either grows: a full-resolution intermediate buffer on a 30 MP image is several hundred MB, so the cache cannot re-establish itself between edits.

The fix

-        return candidate;
+        // Only report a *genuinely new* registration. The caller uses a non-NULL
+        // return to invalidate the pipe cache downstream of the source; returning
+        // `candidate` unconditionally wipes that cache on every history commit.
+        return new ? candidate : NULL;

This is what the function already advertises — its doc comment states it "either returns NULL or the source module."

Correctness

The return value is captured in exactly one place (new_raster, imageop.c:2186) and consumed in exactly one place (the invalidation, imageop.c:2241-2242). The remaining call sites (imageop.c:82, 431, 2330) discard it. Genuine registrations still invalidate: g_hash_table_insert() returns TRUE the first time a module is added as a user, which is precisely the case the invalidation guards.

Note that g_hash_table_insert() reports key novelty, not value change, so a retarget to a different raster_mask_id on an existing source would not invalidate. This cannot occur today: a source exports exactly one raster slot (BLEND_RASTER_ID == 0), so raster_mask_id is constant for a given source.

Reproducing

With a raster mask whose source is early in the pipe and whose consumer is late, run with -d pipe and move any slider:

pipecache invalidate [full HQ]  blend new raster: 7 cachelines after ioporder=2700, blend cache

This repeats on every commit. With the fix it appears once, when the raster user is first registered.

@TurboGit pending @jenshannoschwalm approval, I am flagging this for 5.6.1 as (unless I am barking at the wrong tree, which is always possible when it comes to cache invalidation) it is a genuine bugfix.

Co-authored with Claude.

[jenshannoschwalm]

1. There are many-many invalidations for cachelines and also for rastermasks that should not be necessary now.
2. BUT that rastermask stuff was very errorprone some versions ago - i didn't touch that again sinvce then - and there needs to be a huge lot of testing and very careful reviewing. So i would prefer that for 5.8 although i am very aware of the performance implications for now.

[masterpiga]

Thanks, Hanno, updating the milestone accordingly.

[jenshannoschwalm]

I think this is correct after source reading & checking. I remember working on that code and even doing all the comments why "new" is important :-)

Three thoughts;

1. Would it be easier to understand if we do the invalidation right "here" in dt_iop_commit_blend_params()
2. I might be good to modify the logging code, we might always want to know about the "new" situation and we would ignore reports if not-new?
3. I would prefer a "readable text" in the commit text explaing the why.

[masterpiga]

Thanks, Hanno. Done.

I also added a follow up commit suggested by @kofa73's army of agents.

[jenshannoschwalm]

The more i think about this, the lesser i think it's as trivial as the tag implies :-)

First: i don't think the "simple interactive test" is good enough by far. Multiple masks, en-disabling modules before, between and after raster creators/consumers, toggling HQ mode, mask visualizing passthru, ...

The big question to me is, why do we have to invalidate cachelines at all?
If we have to do it, this would mean the piece hash and thus the cacheline hash is wrong. And it's very likely wrong because we (yet) don't include the status of "module provided a raster mask".

So we should try to get that sorted out. After that, all cacheline invalidations would only be helpful to reduce stress on the caching for a better hit rate.

[masterpiga]

Changed difficulty tag to "average" as this is no longer a one-liner.