Localize image mutex lock into runtime field of Image datablock
ClosedPublic
Actions

Authored by Sergey Sharybin (sergey) on Oct 29 2021, 2:51 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Bastien Montagne (mont29)

Commits

rB9111ea78acf4: Localize image mutex lock into runtime field of Image datablock

Summary

Allows to avoid a global lock being held while reading files from disk,
solving performance issues when Cycles needs to read a lot of packed
images.

Simple test file F11597666

Diff Detail

Repository

rB Blender

Branch

image_runtime_mutex (branched from master)

Build Status

Buildable 18316
Build 18316: arc lint + arc unit

Event Timeline

Sergey Sharybin (sergey) requested review of this revision.Oct 29 2021, 2:51 PM

Sergey Sharybin (sergey) created this revision.

Harbormaster completed remote builds in B18316: Diff 44114.Oct 29 2021, 2:51 PM

Steffen Dünner (SteffenD) added a subscriber: Steffen Dünner (SteffenD).Oct 29 2021, 3:05 PM

Generaly LGTM, main concern here is about potential deadlocks (using several mutexes at the same time is always dangerous...).

source/blender/blenkernel/BKE_image.h
50–53	Not sure this needs to be public API? Afaict this is only used in BKE `image.c`?
source/blender/blenkernel/intern/image.c
5333–5336	not familiar enough with the image pool code, but are we sure this can never deadlock? i would at least add comments about expected order of lock/unlock of pool mutex vs. image mutex.

Sergey Sharybin (sergey) added inline comments.Oct 29 2021, 3:34 PM

source/blender/blenkernel/BKE_image.h
50–53	Was mainly following the Mesh's runtime API. Guess is indeed better to have it a private inside of `image.c`. Will submit a change in a moment!
source/blender/blenkernel/intern/image.c
5333–5336	Pool is never used from within an image cache lock. I'll add a comment.

Make runtime function maipulation private to the image.c
Explain mutex acqusution in the image pool a bit better.

Harbormaster completed remote builds in B18318: Diff 44116.Oct 29 2021, 3:41 PM

Raimund Klink (Raimund58) awarded a token.Oct 29 2021, 4:17 PM

Thanks, LGTM now.

This revision is now accepted and ready to land.Oct 29 2021, 4:28 PM

Seems to work well

test case from this Diff

Master: I1029 09:03:08.657116 12372 session.cpp:467] Total render time: 3.1518 
D13032: I1029 09:04:04.073683 30076 session.cpp:467] Total render time: 1.32681

Test case from D6267

Master: I1029 09:00:46.238179 22708 session.cpp:467] Total render time: 7.56488
D13032: I1029 09:00:22.561700 26416 session.cpp:467] Total render time: 5.96962

Brecht Van Lommel (brecht) accepted this revision.Oct 29 2021, 6:27 PM

Felix Kütt (fkytt) awarded a token.Oct 29 2021, 7:17 PM

kursad k (kursadk) awarded a token.Oct 30 2021, 12:30 AM

Closed by commit rB9111ea78acf4: Localize image mutex lock into runtime field of Image datablock (authored by Sergey Sharybin (sergey)). · Explain WhyNov 1 2021, 12:48 PM

This revision was automatically updated to reflect the committed changes.

Sergey Sharybin (sergey) added a commit: rB9111ea78acf4: Localize image mutex lock into runtime field of Image datablock.