Page MenuHome

BVHCache: Performance
ClosedPublic

Authored by Jeroen Bakker (jbakker) on Fri, May 22, 1:15 PM.

Details

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

Event Timeline

Jeroen Bakker (jbakker) requested review of this revision.Fri, May 22, 1:15 PM
Jeroen Bakker (jbakker) created this revision.
Brecht Van Lommel (brecht) requested changes to this revision.Fri, May 22, 2:45 PM

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.Fri, May 22, 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*?

Jeroen Bakker (jbakker) marked an inline comment as done.

leaner locking mechanism

  • Lazy initialization
Jeroen Bakker (jbakker) planned changes to this revision.EditedTue, May 26, 4:59 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.

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
Jeroen Bakker (jbakker) marked 3 inline comments as done.Thu, May 28, 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
262

Do we need to cast?

  • Cleanup: Small changes
This revision is now accepted and ready to land.Thu, May 28, 6:25 PM
This revision was automatically updated to reflect the committed changes.