Details

Reviewers

Maniphest Tasks

Commits

Summary

This patch changes the BVHCache implementation. It will use
a primitive array in stead of the ListBase. The locking is also changed from a global lock to a per cache instance lock.

The performance of gabby.blend available on the cloud increased from 9.7
fps to 10.5 fps.

Diff Detail

Repository

rB Blender

Branch

bvh-cache-lock (branched from master)

Build Status

Buildable 8264
Build 8264: arc lint + arc unit

Event Timeline

Jeroen Bakker (jbakker) requested review of this revision.May 22 2020, 1:15 PM

Jeroen Bakker (jbakker) created this revision.

Harbormaster completed remote builds in B8162: Diff 25002.May 22 2020, 1:15 PM

Erick Tukuniata (erickblender) awarded a token.May 22 2020, 1:19 PM

Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)May 22 2020, 1:41 PM

Jeroen Bakker (jbakker) added a project: Performance.

Jeroen Bakker (jbakker) added a task: T75207: Profile and optimize modifiers.

Allocating the BVHCache for every mesh seems like a waste of memory.

I also doubt an array is faster than a list here, surely it's the per mesh mutex lock that actually makes the difference? But either list or array fine.

I think you could use mesh_runtime->eval_mutex instead and use that to allocate the BVH cache on demand.

source/blender/blenkernel/intern/mesh_runtime.c
67	Why was this removed? Seems unrelated to the BVH.

This revision now requires changes to proceed.May 22 2020, 2:45 PM

If storage is an array then you can get rid of rather expensive RW mutex and use double-checked lock.

source/blender/blenkernel/intern/bvhutils.c
1598–1600	Is there a specific reason why locking is left up to the caller and not to happen inside of `bvhcache_find` ?
source/blender/makesdna/DNA_mesh_types.h
103–104	Why not to forward-declare `BVHCache` and go away from `void*`?

leaner locking mechanism

Harbormaster completed remote builds in B8223: Diff 25115.May 26 2020, 4:58 PM

Lazy initialization

Harbormaster completed remote builds in B8224: Diff 25116.May 26 2020, 4:58 PM

ASAN fails a cache entry is overwritten. This is introduced by the lazy initialization. The previous commit didn't had this.
Cache should also be able to store NULL as a cache item.

source/blender/blenkernel/intern/bvhutils.c
1598–1600	all the read locks can be removed. the original code used a read/write lock as linked lists cannot be read thread safe. but now we could use a different synchronization mechanism.
source/blender/makesdna/DNA_mesh_types.h
103–104	the BVHCache contains pointers to `BVHTree` what is defined in the kernel. I don't think it is good to move the BVHTree to DNA, but we could introduce void* inside the BVHCache for that. This solves the additional allocation/free for the cache.

Sergey Sharybin (sergey) added inline comments.May 26 2020, 5:34 PM

source/blender/makesdna/DNA_mesh_types.h
103–104	Not saying to move `BVHCache` to DNA, saying to forward-declare it: ... struct BVHCache; ... typedef struct Mesh_Runtime { ... struct BVHCache *bvh_cache; ... };

Lazy initialization
Support to cache NULL as a valid item, Fixed overwrite tree
Forward declaration of the BVH Cache

Harbormaster completed remote builds in B8263: Diff 25191.May 28 2020, 2:27 PM

Jeroen Bakker (jbakker) marked 3 inline comments as done.May 28 2020, 2:36 PM

Jeroen Bakker (jbakker) added inline comments.

source/blender/blenkernel/intern/bvhutils.c
144	!item->is_filled
source/blender/blenkernel/intern/mesh_runtime.c
76	move to line 70
source/blender/editors/transform/transform_snap_object.c
251	Do we need to cast?