Pixelpipe caching finally fixed (very hopefully) #14130
Merged
Conversation
This file contains 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
jenshannoschwalm
added
bugfix
jenshannoschwalm
force-pushed
the
pixelpipe_cache_redone
branch
from
06b004e
to
fd7e744
Compare
|
Rebased to master and minor code cleanup, from my side a ready-to-go There is some work pending about #11069 not for this , it needs more testing ... |
The pixelpipe caching worked flawlessly as long as no mask_display was involved, that is defined in the pipe struct as mask_display. It is set whenever we want some "special things" like masks visualized on the display, the gamma module is responsible for this. The pixelpipe cache makes sure the data are valid by using a hash. Whenever we start processing a module we get a buffer (cacheline) via dt_dev_pixelpipe_cache_get() and keep the hash in the caching structs. The buffer is used for the module's processed **output**. If we restart the pixelpipe, we check for existing cacheline buffers with the same hash and take that buffer as granted output of a module and avoid recalculating the whole pixelpipe until that point. So - what's wrong with that / what did me take so long to understand? BTW, that's not new in the improved pixelpipe cache ... The big point is, **we can and do change** the value of mask_display while processing the pixelpipe at every stage. So we generate new data in the cacheline but as this data change is not refelected by an updated hash we later might / certainly will re-use wrong data as valid! How has this been fixed? 1. Whenever we are in mask_display mode we don't get a "regular" cacheline but use only the first two cachelines by swapping (as we do for exports). This is nice as we do not process most modules in mask_display mode but can do a shortcut. This way we reduce the cacheload and keep cachelines from non-mask_display mode valid, both lead to an improved hit rate. 2. The pixelpipe cache keeps track of mask_display mode and always sets dummy hashes so those lines will never be used by the regular pixelpipe. 3. After processing a module we take care of a change in mask_display. If in mask_display mode we have to in-validate the cacheline, dt_dev_pixelpipe_cache_unweight() is used for that. 4. The house-keeping of the cache had to be modified slightly to allow the "swapping" mode described above. While being here - imgid is now always int32_t - many const added - in some functions we had a parameter 'module', renamed to 'position' for understanding code - simplified the next_important and important hint tracking - slightly modified some debug info and added info about the mask_display
focus_hash and history_item->enabled are functionally both gboolean
TurboGit
reviewed
Apr 9, 2023
The pixelpipe cache requires at least 2 cachelines in all cases for swapping in & output of a module. If we avoid cache data usage like in display_mask mode we also do the swapping. So housekeeping for **used cachelines** is limited. In all those case we use DT_PIPECACHE_MIN for code maintenance.
jenshannoschwalm
force-pushed
the
pixelpipe_cache_redone
branch
from
fd7e744
to
abb7fb8
Compare
TurboGit
reviewed
Apr 9, 2023
jenshannoschwalm
added a commit
to jenshannoschwalm/darktable
that referenced
this pull request
Apr 10, 2023
For an overview about "how the pixelpipe cache works" see darktable-org#14130 There has been an issue related to some modules internally showing histograms (hist-module), the underlying reasons are 1. those histograms are updated **while** processing the module 2. if we develop an image in darkroom we often do **not** process the whole pixelpipe but take data from the pixelpipe cache thus the histogram updating as in (1) is not done. We have discussed basically two options until now 1. do some sort of pre-processing before executing the pipe 2. introduce some special widget updating code both seem to be pretty difficult and likely over-designed. This pr follows a different approach: 1. If we develop an image with one of the hist-modules **expanded** we accept some performance penalty and force the pipeline to be processed from the first expanded hist-module. As these modules are late in the pipe the penalty is pretty small. 2. A new mask `DT_REQUEST_EXPANDED` in `dt_dev_request_flags_t` takes care of this. The modules in question set this flag. 3. In `dt_dev_pixelpipe_t` we have a new gboolean `nocache`, this flag is checked in the pixelpipe and it's caching code. If **TRUE**, the requested cachelines are marked as invalid. (This is like we do for visualizing masks in some ways) 4. While processing each module in the **full** pixelpipe we check for a) the status of `DT_REQUEST_EXPANDED` b) the module being enabled & expanded and possibly unvalidate the current cacheline and set the pixelpipe `nocache` flag. Fixes darktable-org#11069 Likely Fixes darktable-org#11003
jenshannoschwalm
added a commit
to jenshannoschwalm/darktable
that referenced
this pull request
Apr 10, 2023
For an overview about "how the pixelpipe cache works" see darktable-org#14130 There has been an issue related to some modules internally showing histograms (hist-module), the underlying reasons are 1. those histograms are updated **while** processing the module 2. if we develop an image in darkroom we often do **not** process the whole pixelpipe but take data from the pixelpipe cache thus the histogram updating as in (1) is not done. We have discussed basically two options until now 1. do some sort of pre-processing before executing the pipe 2. introduce some special widget updating code both seem to be pretty difficult and likely over-designed. This pr follows a different approach: 1. If we develop an image with one of the hist-modules **expanded** we accept some performance penalty and force the pipeline to be processed from the first expanded hist-module. As these modules are late in the pipe the penalty is pretty small. 2. A new mask `DT_REQUEST_EXPANDED` in `dt_dev_request_flags_t` takes care of this. The modules in question set this flag. 3. In `dt_dev_pixelpipe_t` we have a new gboolean `nocache`, this flag is checked in the pixelpipe and it's caching code. If **TRUE**, the requested cachelines are marked as invalid. (This is like we do for visualizing masks in some ways) 4. While processing each module in the **full** pixelpipe we check for a) the status of `DT_REQUEST_EXPANDED` b) the module being enabled & expanded and possibly unvalidate the current cacheline and set the pixelpipe `nocache` flag. Fixes darktable-org#11069 Likely Fixes darktable-org#11003
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.
A short summary how pixelpipe caching works
The pixelpipe caching worked flawlessly as long as no mask_display was involved, that is defined in the pipe struct as mask_display.
It is set whenever we want some "special things" like masks visualized on the display, the gamma module is responsible for this.
The pixelpipe cache makes sure the data are valid by using a hash. Whenever we start processing a module we get a buffer (cacheline) via dt_dev_pixelpipe_cache_get() and keep the hash in the caching structs. The buffer is used for the module's processed output.
If we restart the pixelpipe, we check for existing cacheline buffers with the same hash and take that buffer as granted output of a module and avoid recalculating the whole pixelpipe until that point.
The problem
So - what's wrong with that / what did me take so long to understand? BTW, that's not new in the improved pixelpipe cache ...
The big point is, we can and do change the value of mask_display while processing the pixelpipe at every stage.
So we generate new data in the cacheline but as this data change is not refelected by an updated hash, we later might / certainly will re-use wrong data as valid!
How has this been fixed?
While being here