Page MenuHome
Differential D15415

Undo: Improve image undo performance
ClosedPublic
Actions

Authored by Alex Parker (zanders3) on Jul 10 2022, 6:59 PM.

Details

Reviewers
Jeroen Bakker (jbakker)
Maniphest Tasks
T99546: UNDO: Image undo performance
Commits
rB44258b5ad073: Undo: Improve image undo performance
Summary

When texture painting a lot of time is spent in ED_image_paint_tile_find.
This fixes stores the PaintTiles in a blender::Map making ED_image_paint_tile_find an O(1) rather than O(n) operation.

When using threading the locking should happen during read as well,
still this gives a boost in performance as the read is now much faster.

Diff Detail

Repository
rB Blender
Branch
paint-tile-perf (branched from master)
Build Status
Buildable 23004
Build 23004: arc lint + arc unit

Event Timeline

Alex Parker (zanders3) requested review of this revision.Jul 10 2022, 6:59 PM
Alex Parker (zanders3) created this revision.
Harbormaster completed remote builds in B22928: Diff 53487.Jul 10 2022, 6:59 PM
Alex Parker (zanders3) added a task: T99546: UNDO: Image undo performance.Jul 10 2022, 7:02 PM
Jeroen Bakker (jbakker) awarded a token.Jul 10 2022, 8:25 PM
Evan Wilson (EAW) awarded a token.Jul 11 2022, 1:52 AM
Evan Wilson (EAW) added a subscriber: Evan Wilson (EAW).
Jeroen Bakker (jbakker) added a comment.Jul 11 2022, 7:47 AM

Thanks for the patch. Seems mostly like it needs some code style changes. Still want to look into compiler warnings on other systems.

I will do some code review and testing later today. I saw you tested it with 2d painting. I believe the bottleneck is with 3d texture painting on large images. I can provide the figures that can be mentioned in the description.

source/blender/editors/include/ED_paint.h
80

Believe PaintTileMap struct should be predefined.

source/blender/editors/space_image/image_undo.cc
129

Should be a CPP struct.

368

Use CPP casting (static_cast<UndoImageTile*>(...))
Also in rest of the file.

373

I beleive these should be uint32_t

733

As PaintTileMap is cpp and requires construction it might be better to use a pointer. This way the initialization doesn't need to use a not common contructor initialization, but the more familiar MEM_new

Evan Wilson (EAW) added inline comments.Jul 11 2022, 10:47 AM
source/blender/editors/space_image/image_undo.cc
1076–1078

Space preceding is Blender's style

Jeroen Bakker (jbakker) added inline comments.Jul 11 2022, 11:17 AM
source/blender/editors/space_image/image_undo.cc
92

Use CPP struct

368

Perhaps make UndoImageTile a CPP struct and use MEM_new.

733

Or make ImageUndoStep a CPP struct and use MEM_new

Jeroen Bakker (jbakker) added a reviewer: Jeroen Bakker (jbakker).Jul 11 2022, 12:15 PM
Jeroen Bakker (jbakker) requested changes to this revision.Jul 11 2022, 12:38 PM

Without patch applied

With patch applied

source/blender/editors/space_image/image_undo.cc
120

On linux this doesn't compile. Would rename it to uint_ptr and perhaps use uint32_t* as type.

/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:346:11: error: declaration of ‘uint* UndoImageTile::<unnamed union>::uint’ changes meaning of ‘uint’ [-fpermissive]
  346 |     uint *uint;
      |           ^~~~
In file included from /usr/include/stdlib.h:394,
                 from /usr/include/c++/11/cstdlib:75,
                 from /usr/include/c++/11/stdlib.h:36,
                 from /home/jeroen/blender-git/blender/source/blender/blenlib/BLI_blenlib.h:36,
                 from /home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:24:
/usr/include/x86_64-linux-gnu/sys/types.h:150:22: note: ‘uint’ declared here as ‘typedef unsigned int uint’
139–140

This function isn't used. Not sure if that is a bug... (not related to this patch) I will have a look I might have introduced the bug in 3.2....

360

Doesn't compile on linux. See other comment.

This revision now requires changes to proceed.Jul 11 2022, 12:38 PM
Jeroen Bakker (jbakker) added inline comments.Jul 11 2022, 7:37 PM
source/blender/editors/space_image/image_undo.cc
139–140

This is a local bug in this patch. The elements inside the map aren't freed correctly.
Perhaps move this to a destructor of PaintTile?

1010

map contains elements that arent (fully) freed here.
call ptile_free_map

Alex Parker (zanders3) updated this revision to Diff 53563.Jul 12 2022, 9:45 PM
Alex Parker (zanders3) marked 14 inline comments as done.
  • Cleanup: Make things more cpp style. Fix memory leak on PaintTileMap destruction.

Hopefully fix the linux compile.

Harbormaster completed remote builds in B22961: Diff 53563.Jul 12 2022, 9:45 PM
Alex Parker (zanders3) added inline comments.Jul 12 2022, 9:45 PM
source/blender/editors/space_image/image_undo.cc
733

I think I can't do that because ImageUndoStep is allocated by the undo system outside of this file.

I agree making it a pointer is probably a better choice so I'll do that. Could always go back to placement new construction if worried about pointer chasing.

Alex Parker (zanders3) updated this revision to Diff 53564.Jul 12 2022, 9:49 PM
  • Cleanup: Missed a static_cast
Harbormaster completed remote builds in B22962: Diff 53564.Jul 12 2022, 9:49 PM
Jeroen Bakker (jbakker) added a comment.Jul 13 2022, 12:15 PM

Thanks fore the update. I do think we are almost there.
Besides the mentioned struct keyword there are some compiler warnings that should be minimized if possible.

/home/jeroen/blender-git/blender/source/blender/editors/include/ED_paint.h:88:7: warning: function 'ED_image_paint_tile_push' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
void *ED_image_paint_tile_push(struct PaintTileMap *paint_tiles,
      ^
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:191:7: note: the definition seen here
void *ED_image_paint_tile_push(PaintTileMap *paint_tile_map,
      ^
/home/jeroen/blender-git/blender/source/blender/editors/include/ED_paint.h:88:7: note: differing parameters are named here: ('paint_tiles'), in definition: ('paint_tile_map')
void *ED_image_paint_tile_push(struct PaintTileMap *paint_tiles,
      ^                                             ~~~~~~~~~~~
                                                    paint_tile_map
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:86:37: warning: redundant void argument list in function definition [modernize-redundant-void-arg]
static ImBuf *imbuf_alloc_temp_tile(void)
                                    ^~~~
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:188:10: warning: use nullptr [modernize-use-nullptr]
  return NULL;
         ^~~~
         nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:203:47: warning: use nullptr [modernize-use-nullptr]
  const bool has_float = (ibuf->rect_float != NULL);
                                              ^~~~
                                              nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:286:64: warning: use nullptr [modernize-use-nullptr]
    ImBuf *ibuf = BKE_image_acquire_ibuf(image, &ptile->iuser, NULL);
                                                               ^~~~
                                                               nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:287:49: warning: use nullptr [modernize-use-nullptr]
    const bool has_float = (ibuf->rect_float != NULL);
                                                ^~~~
                                                nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:323:41: warning: use nullptr [modernize-use-nullptr]
    BKE_image_release_ibuf(image, ibuf, NULL);
                                        ^~~~
                                        nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:420:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
typedef struct UndoImageBuf {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:463:53: warning: use nullptr [modernize-use-nullptr]
  ubuf->image_state.use_float = ibuf->rect_float != NULL;
                                                    ^~~~
                                                    nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:499:28: warning: use nullptr [modernize-use-nullptr]
  if ((ibuf->rect_float != NULL) && (ubuf->image_state.use_float == false)) {
                           ^~~~
                           nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:539:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
typedef struct UndoImageHandle {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:565:61: warning: use nullptr [modernize-use-nullptr]
    ImBuf *ibuf = BKE_image_acquire_ibuf(image, &uh->iuser, NULL);
                                                            ^~~~
                                                            nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:566:26: warning: use nullptr [modernize-use-nullptr]
    if (UNLIKELY(ibuf == NULL)) {
                         ^~~~
                         nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:602:41: warning: use nullptr [modernize-use-nullptr]
    BKE_image_release_ibuf(image, ibuf, NULL);
                                        ^~~~
                                        nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:636:10: warning: use nullptr [modernize-use-nullptr]
  return NULL;
         ^~~~
         nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:645:16: warning: use nullptr [modernize-use-nullptr]
  ubuf->post = NULL;
               ^~~~
               nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:653:15: warning: use nullptr [modernize-use-nullptr]
  if (ubuf == NULL) {
              ^~~~
              nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:668:10: warning: use nullptr [modernize-use-nullptr]
  return NULL;
         ^~~~
         nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:678:10: warning: use nullptr [modernize-use-nullptr]
  return NULL;
         ^~~~
         nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:687:21: warning: use nullptr [modernize-use-nullptr]
  uh->iuser.scene = NULL;
                    ^~~~
                    nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:695:13: warning: use nullptr [modernize-use-nullptr]
  if (uh == NULL) {
            ^~~~
            nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:734:18: warning: use nullptr [modernize-use-nullptr]
  if (uh_prev != NULL) {
                 ^~~~
                 nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:744:10: warning: use nullptr [modernize-use-nullptr]
  return NULL;
         ^~~~
         nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:806:26: warning: use nullptr [modernize-use-nullptr]
        ptile->rect.pt = NULL;
                         ^~~~
                         nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:819:77: warning: use nullptr [modernize-use-nullptr]
        ImBuf *ibuf = BKE_image_acquire_ibuf(uh->image_ref.ptr, &uh->iuser, NULL);
                                                                            ^~~~
                                                                            nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:836:31: warning: use nullptr [modernize-use-nullptr]
                              NULL);
                              ^~~~
                              nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:844:38: warning: use nullptr [modernize-use-nullptr]
              if ((ubuf_reference != NULL) && ((ubuf_pre->tiles[i] == NULL) ||
                                     ^~~~
                                     nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:844:71: warning: use nullptr [modernize-use-nullptr]
              if ((ubuf_reference != NULL) && ((ubuf_pre->tiles[i] == NULL) ||
                                                                      ^~~~
                                                                      nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:848:43: warning: use nullptr [modernize-use-nullptr]
                if (ubuf_pre->tiles[i] != NULL) {
                                          ^~~~
                                          nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:852:40: warning: use nullptr [modernize-use-nullptr]
                  ubuf_pre->tiles[i] = NULL;
                                       ^~~~
                                       nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:871:43: warning: use nullptr [modernize-use-nullptr]
                if (ubuf_pre->tiles[i] != NULL) {
                                          ^~~~
                                          nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:889:57: warning: use nullptr [modernize-use-nullptr]
        BKE_image_release_ibuf(uh->image_ref.ptr, ibuf, NULL);
                                                        ^~~~
                                                        nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:981:60: warning: use nullptr [modernize-use-nullptr]
    ED_object_mode_set_ex(C, OB_MODE_TEXTURE_PAINT, false, NULL);
                                                           ^~~~
                                                           nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:1068:17: warning: use nullptr [modernize-use-nullptr]
  bContext *C = NULL; /* special case, we never read from this. */
                ^~~~
                nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:1099:50: warning: use nullptr [modernize-use-nullptr]
                                                 NULL);
                                                 ^~~~
                                                 nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:1116:33: warning: use nullptr [modernize-use-nullptr]
  BKE_undosys_step_push(ustack, NULL, NULL);
                                ^~~~
                                nullptr
/home/jeroen/blender-git/blender/source/blender/editors/space_image/image_undo.cc:1116:39: warning: use nullptr [modernize-use-nullptr]
  BKE_undosys_step_push(ustack, NULL, NULL);
                                      ^~~~
                                      nullptr
source/blender/editors/sculpt_paint/paint_image_2d.c
1199

Don't think struct keyword is needed. It is predefined in the header.

Just pointing out the first occurrence. Please check all usages.

source/blender/editors/space_image/image_undo.cc
141

For clarity it would be better that this map didn't contain a ptr. the PaintTile is owned by the map, by using a pointer this isn't clear to a developer reading this code.
It also improves performance a bit as the primary data isn't scattered around in memory. But for safety I would keep this as a pointer for now.

Currently (doesn't happen to my knowledge) but C code could request a ptr add new PaintTiles which invalidates the already given pointer.

Jeroen Bakker (jbakker) added a comment.Jul 13 2022, 12:19 PM

Note when using 3d texture painting in sculpt mode (experimental feature) the spinlock isn't working. Looking at the code it doesn't seem to be thread safe as add_new has undefined behavior when the key already exists. Perhaps it fails to release the spinlock

Jeroen Bakker (jbakker) requested changes to this revision.Jul 13 2022, 12:23 PM
Jeroen Bakker (jbakker) added inline comments.
source/blender/editors/space_image/image_undo.cc
280

Although inside a lock, this isn't thread safe. when doing 3d texture painting in sculpt mode (experimental feature) the lock isn't released correctly when (untested) multiple threads are adding data for the same key. To test enable sculpe mode textureing as experimental feature and try to do a painting operation.

If you need some help to set it up, you can always ask in the sculp-texture-paint-module chat room.

This revision now requires changes to proceed.Jul 13 2022, 12:23 PM
Alex Parker (zanders3) added a comment.EditedJul 15 2022, 1:06 AM

re: the linux warnings I have installed Ubuntu to make sure future revisions compile ok without warnings on linux.

I think I've fixed the race condition in ED_image_paint_tile_push by adding a lock inside ED_image_paint_tile_find and also handling the case where two threads have created a paint tile in parallel.
This should fix the undefined behavior at least - but I'm wondering if I should remove the find_prev option since now we can't support storing multiple images under the same 'key'.

I also managed to do some testing of the new experimental sculpt painting mode and it seems to be working and no longer crashes.

source/blender/editors/space_image/image_undo.cc
141

I agree I did start making this change but unfortunately ED_image_paint_tile_push takes a bool **r_valid which points to the valid field in a PaintTile, forcing me to alloc that on the heap so it doesn't move around.

It looks like ProjPaintImage *pjIma stores an array of bool and ushort pointers which point into a PaintTile whilst painting occurs.

I could look into changing that but worried about the impact of that change!

Alex Parker (zanders3) updated this revision to Diff 53666.Jul 15 2022, 1:09 AM
Alex Parker (zanders3) marked an inline comment as done.
  • Add spin locks and fix duplicate paint tile race condition.
Harbormaster completed remote builds in B23004: Diff 53666.Jul 15 2022, 1:09 AM
Jeroen Bakker (jbakker) requested changes to this revision.Jul 15 2022, 10:26 AM
Jeroen Bakker (jbakker) added inline comments.
source/blender/editors/space_image/image_undo.cc
218

In stead of doing locking, unlocking with a gab in between we should do a single lock in this function and not pass it to ED_image_paint_tile_find.
Reason for this is that there is still some lines of code that aren't guarded with the lock that could still lead to threading issues. It currently doesn't crash anymore but there might be higher change of overallocating buffers that are freed because of this gab.

So what I want to suggest is to move the BLI_spin_lock(&paint_tiles_lock); at the start of this function and move the BLI_spin_unlock(&paint_tiles_lock); to all exit points of this function.
I want to go over a better locking mechanism here, but that should not be part of this patch. Another idea was to use double locking, but that cannot work in this case as reading would still require a lock.

This revision now requires changes to proceed.Jul 15 2022, 10:26 AM
Alex Parker (zanders3) updated this revision to Diff 53781.Jul 18 2022, 10:28 PM
  • Lock paint_tiles_lock during buffer creation
Harbormaster completed remote builds in B23052: Diff 53781.Jul 18 2022, 10:28 PM
Alex Parker (zanders3) marked an inline comment as done.Jul 18 2022, 10:31 PM

Hopeful this resolves all the remaining issues. Happy to go further with any locking changes you suggest if you wish :)

I have tested sculpture paint, 2D painting and projective painting and all seem ok with the locking changes.

Jeroen Bakker (jbakker) added a comment.Jul 18 2022, 11:19 PM

I am not able to reply in the next week. Hope it is ok?

Jeroen Bakker (jbakker) accepted this revision.Jul 25 2022, 8:12 AM
Jeroen Bakker (jbakker) retitled this revision from Fix T99546: Improve image undo performance to Undo: Improve image undo performance.
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)

Seems all good now. Thanks for the patch!

This revision is now accepted and ready to land.Jul 25 2022, 8:13 AM
Closed by commit rB44258b5ad073: Undo: Improve image undo performance (authored by Alex Parker (zanders3), committed by Jeroen Bakker (jbakker)). · Explain WhyJul 25 2022, 8:14 AM
This revision was automatically updated to reflect the committed changes.
Jeroen Bakker (jbakker) added a commit: rB44258b5ad073: Undo: Improve image undo performance.

Revision Contents
Changeset List

PathSize
source/
blender/
editors/
include/
10 lines
sculpt_paint/
2 lines
6 lines
2 lines
2 lines
space_image/
2 lines
image_undo.cc
image_undo.c
318 lines

Diff 53666

View Options

source/blender/editors/include/ED_paint.h

Loading...
View Options

source/blender/editors/sculpt_paint/paint_image.cc

Loading...
View Options

source/blender/editors/sculpt_paint/paint_image_2d.c

Loading...
View Options

source/blender/editors/sculpt_paint/paint_image_proj.c

Loading...
View Options

source/blender/editors/sculpt_paint/sculpt_paint_image.cc

Loading...
View Options

source/blender/editors/space_image/CMakeLists.txt

Loading...
View Options

source/blender/editors/space_image/image_undo.c

Loading...
View Options

source/blender/editors/space_image/image_undo.cc

Loading...