blender
/
blender
136
406
Fork 589
Code Issues 6.3k Pull Requests 766 Projects 19 Wiki Activity

Use hash for Collection.gobject lookup, speedup object linking #104553

Closed
Campbell Barton wants to merge 5 commits from ideasman42:pr-collection-object-hash into main
pull from: ideasman42:pr-collection-object-hash
merge into: blender:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 6 Commits 5 Files Changed 2 +12 -9
1 changed files with 12 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 028b6e06a2 - Show all commits

21
source/blender/blenkernel/intern/collection.c
View File

@ -67,8 +67,10 @@ static bool collection_child_remove(Collection *parent, Collection *collection);
static bool collection_object_add(
Main *bmain, Collection *collection, Object *ob, int flag, const bool add_us);
static void collection_object_remove_no_gobject_hash(
Main *bmain, Collection *collection, Object *ob, CollectionObject *cob, const bool free_us);
static void collection_object_remove_no_gobject_hash(Main *bmain,
Collection *collection,
CollectionObject *cob,
const bool free_us);
static bool collection_object_remove(Main *bmain,
Collection *collection,
Object *ob,
@ -565,7 +567,7 @@ bool BKE_collection_delete(Main *bmain, Collection *collection, bool hierarchy)
/* Remove child objects. */
CollectionObject *cob = collection->gobject.first;
while (cob != NULL) {
collection_object_remove_no_gobject_hash(bmain, collection, cob->ob, cob, true);
collection_object_remove_no_gobject_hash(bmain, collection, cob, true);
cob = collection->gobject.first;
}
@ -594,7 +596,7 @@ bool BKE_collection_delete(Main *bmain, Collection *collection, bool hierarchy)
}
/* Remove child object. */
collection_object_remove_no_gobject_hash(bmain, collection, cob->ob, cob, true);
collection_object_remove_no_gobject_hash(bmain, collection, cob, true);
cob = collection->gobject.first;
}
}
@ -1144,12 +1146,13 @@ static bool collection_object_add(
/**
* A version of #collection_object_remove that does not handle `collection->runtime.gobject_hash`,
* Either the caller must have removed the object from the hash or the hash may be NULL.
ideasman42 marked this conversation as resolved
Sergey Sharybin commented 2023-02-20 16:13:08 +01:00
Review
Copy Link

This is something very surprising to me. I am not sure why this is the case, but it definitely asks to be written down in the comment here.

From quick look around the collection_object_remove_no_gobject_hash is either called with ob=cob->ob, and in other cases is called with a result of ghash lookup. The latter one is done after collection_gobject_hash_ensure which seems to assert that cob->ob is the same as ob.

In general, always document such cases for the future reader. It saves a lot of time (for both current review, and future you and future developers looking into the code).

This is something very surprising to me. I am not sure why this is the case, but it definitely asks to be written down in the comment here. From quick look around the `collection_object_remove_no_gobject_hash` is either called with ob=cob->ob, and in other cases is called with a result of ghash lookup. The latter one is done after `collection_gobject_hash_ensure` which seems to assert that `cob->ob` is the same as `ob`. In general, always document such cases for the future reader. It saves a lot of time (for both current review, and future you and future developers looking into the code).
Campbell Barton commented 2023-02-21 12:12:10 +01:00
Review
Copy Link

This must have been a use-after-free mistake, double checked with the ob argument removed and the tests now pass.

This must have been a use-after-free mistake, double checked with the `ob` argument removed and the tests now pass.
*
* \note in rare cases `cob->ob != ob`, so both arguments need to be passed in.
*/
static void collection_object_remove_no_gobject_hash(
Main *bmain, Collection *collection, Object *ob, CollectionObject *cob, const bool free_us)
static void collection_object_remove_no_gobject_hash(Main *bmain,
Collection *collection,
ideasman42 marked this conversation as resolved
Bastien Montagne commented 2023-02-16 09:42:28 +01:00
Review
Copy Link

This call can be removed (see comment in collection_gobject_hash_ensure).

This call can be removed (see comment in `collection_gobject_hash_ensure`).
CollectionObject *cob,
const bool free_us)
{
Object *ob = cob->ob;
BLI_freelinkN(&collection->gobject, cob);
ideasman42 marked this conversation as resolved
Bastien Montagne commented 2023-02-13 14:52:27 +01:00
Review
Copy Link

Couldn't this use instead cob = BLI_ghash_popkey(collection->runtime.gobject_hash, ob, NULL);? Then there is no need for an extra lookup below in the call to collection_gobject_hash_remove_object(collection, ob);

Couldn't this use instead `cob = BLI_ghash_popkey(collection->runtime.gobject_hash, ob, NULL);`? Then there is no need for an extra lookup below in the call to `collection_gobject_hash_remove_object(collection, ob);`
Campbell Barton commented 2023-02-16 05:01:39 +01:00
Review
Copy Link

Yep, good idea.

Yep, good idea.
BKE_collection_object_cache_free(collection);
@ -1174,7 +1177,7 @@ static bool collection_object_remove(Main *bmain,
if (cob == NULL) {
return false;
}
collection_object_remove_no_gobject_hash(bmain, collection, ob, cob, free_us);
collection_object_remove_no_gobject_hash(bmain, collection, cob, free_us);
return true;
}