Darktable 5.0 pipe cache fixes #17906
Conversation
In dt 5.0 the iop_order of a module no longer is the n-th position in the pipe but an int derived from the original double iop_order allowing safe reordering of pipe modules. dt_dev_pixelpipe_cache_hash() has been fixed to respect this, also only enabled modules must be included in the hash. It's callers and callers of dt_dev_pixelpipe_cache_available() and dt_dev_pixelpipe_cache_get() had to be checked as they make use of the hash. Note: this also fixes internal hash calculation in tone equalizer. We should keep the last recorded module for far improved cache hits, it doesn't make sense UI-wise to clear this at all. This is important for pixelpipe cache performance especially if used via shortcuts, (midi) input devices; master was code was bad and drastically decreased cache hit rates. While distorting the raster mask we must only call the module distort function if the module is enabled for correctness (and performance).
1. added some const where possible 2. Some reformatting for common code style and for better support for grep and editor syntax support. 3. Some cl_int instead of int where formally correct 4. avoid some casts
jenshannoschwalm
added
bugfix
|
I'm not sure it's fully effective. This is with a 10 MPx image, and 5 instances of diffuse or sharpen (for test), then toggling filmic rgb a few times. Look for the |
|
It's not "fully effective", right - there are some points that need more love and could lead to improved UI experience. This one takes care only for those parts that are safe as not introducing anything new as targeted still for 5.0 |
|
Right, just wanted to add quick feedback. After the 2nd run, it settles down; for example, dragging sliders in local contrast at the end of the pipeline is pretty much instantaneous. |
|
There will very likely be more PR's following pretty soon after 5.0 :-) |
jenshannoschwalm
added
the
scope: performance
@TurboGit i know it's very late in the 5.0 dev cycle but the first commit is really bugfixing and taking care of a bad UI performance regression so i guess we need a good review and bite a bullet. (I have been testing this for the last three days including "crazy" raster and details masks work and am confident it's all good.)
I started investigating that a) from #17641 reported by @kofa73, b) the observed-here clearly bad cache performance and c) situations where we definitely should have the cache available but didn't due to wrong calculation.
Fixes #17641
from the first/main commit (the second commit is just code readability so optional for 5.0):
Fix pixelpipe cache and hash for new iop_order mechanism
In dt 5.0 the iop_order of a module no longer is the n-th position in the pipe but an int derived from
the original double iop_order allowing safe reordering of pipe modules.
dt_dev_pixelpipe_cache_hash()has been fixed to respect this, also only enabled modules must be included in the hash.It's callers and callers of
dt_dev_pixelpipe_cache_available()anddt_dev_pixelpipe_cache_get()had to be checked as they make use of the hash.Note: this also fixes internal hash calculation in tone equalizer.
We should keep the last recorded module for far improved cache hits, it doesn't make sense UI-wise to
clear this at all. This is important for pixelpipe cache performance especially if used via shortcuts,
(midi) input devices; master was code was bad and drastically decreased cache hit rates.
While distorting the raster mask we must only call the module distort function if the module is enabled
for correctness (and performance).