griefith/test2
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Refactor: IDProperty: accelerate looking up properties by name

Browse Source 
Currently, `IDP_GetPropertyFromGroup` is a common bottleneck because it has to
iterate over all properties to find the one with the right name.

This patch adds a hash table to id property groups that finding properties by
name in O(1) instead of O(n). The main tricky aspect is to find all the places
where this needs to be created/updated/freed. I tried to find all places but if
I missed some place, it should be easy to fix.

Pull Request: https://projects.blender.org/blender/blender/pulls/140907
This commit is contained in:
Jacques Lucke
2025-09-12 11:34:38 +02:00
parent e246838639
commit 6cb2226f13
6 changed files with 162 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
source/blender/blenkernel/BKE_idprop.hh
View File

@@ -10,6 +10,7 @@
#include <memory>
#include "DNA_ID.h"
#include "DNA_ID_enums.h"
#include "BLI_compiler_attrs.h"
@@ -17,6 +18,7 @@
#include "BLI_span.hh"
#include "BLI_string_ref.hh"
#include "BLI_sys_types.h"
#include "BLI_vector_set.hh"
struct BlendDataReader;
struct BlendWriter;
@@ -170,12 +172,6 @@ void IDP_MergeGroup_ex(IDProperty *dest, const IDProperty *src, bool do_overwrit
* and free the property.
*/
bool IDP_AddToGroup(IDProperty *group, IDProperty *prop) ATTR_NONNULL();
/**
* This is the same as IDP_AddToGroup, only you pass an item
* in the group list to be inserted after.
*/
bool IDP_InsertToGroup(IDProperty *group, IDProperty *previous, IDProperty *pnew)
ATTR_NONNULL(1 /*group*/, 3 /*pnew*/);
/**
* \note this does not free the property!
*
@@ -191,13 +187,6 @@ void IDP_FreeFromGroup(IDProperty *group, IDProperty *prop) ATTR_NONNULL();
IDProperty *IDP_GetPropertyFromGroup(const IDProperty *prop,
blender::StringRef name) ATTR_WARN_UNUSED_RESULT
ATTR_NONNULL();
/**
* This is a slightly more efficient version of the function above in the when there are lots of
* properties. It can be faster because it avoids computing the length of everything that the
* string is compared to. Also see #140706.
*/
IDProperty *IDP_GetPropertyFromGroup(const IDProperty *prop,
const char *name) ATTR_WARN_UNUSED_RESULT ATTR_NONNULL();
/**
* Same as #IDP_GetPropertyFromGroup but ensure the `type` matches.
*/
@@ -478,6 +467,17 @@ class IDPropertyDeleter {
}
};
struct IDPropertyGroupChildrenSet {
struct IDPropNameGetter {
StringRef operator()(const IDProperty *value) const
{
return StringRef(value->name);
}
};
CustomIDVectorSet<IDProperty *, IDPropNameGetter, 8> children;
};
/** \brief Allocate a new IDProperty of type IDP_BOOLEAN, set its name and value. */
std::unique_ptr<IDProperty, IDPropertyDeleter> create_bool(StringRef prop_name,
bool value,

1
source/blender/blenkernel/CMakeLists.txt
View File

@@ -825,6 +825,7 @@ if(WITH_GTESTS)
intern/file_handler_test.cc
intern/grease_pencil_test.cc
intern/idprop_serialize_test.cc
intern/idprop_test.cc
intern/image_partial_update_test.cc
intern/image_test.cc
intern/lattice_deform_test.cc

92
source/blender/blenkernel/intern/idprop.cc
View File

@@ -142,6 +142,14 @@ void IDP_AppendArray(IDProperty *prop, IDProperty *item)
IDP_SetIndexArray(prop, prop->len - 1, item);
}
static void idp_group_children_map_ensure(IDProperty &prop)
{
BLI_assert(prop.type == IDP_GROUP);
if (!prop.data.children_map) {
prop.data.children_map = MEM_new<IDPropertyGroupChildrenSet>(__func__);
}
}
void IDP_ResizeIDPArray(IDProperty *prop, int newlen)
{
BLI_assert(prop->type == IDP_IDPARRAY);
@@ -565,11 +573,10 @@ static IDProperty *IDP_CopyGroup(const IDProperty *prop, const int flag)
{
BLI_assert(prop->type == IDP_GROUP);
IDProperty *newp = idp_generic_copy(prop, flag);
newp->len = prop->len;
newp->subtype = prop->subtype;
LISTBASE_FOREACH (IDProperty *, link, &prop->data.group) {
BLI_addtail(&newp->data.group, IDP_CopyProperty_ex(link, flag));
IDP_AddToGroup(newp, IDP_CopyProperty_ex(link, flag));
}
return newp;
@@ -581,8 +588,7 @@ void IDP_SyncGroupValues(IDProperty *dest, const IDProperty *src)
BLI_assert(src->type == IDP_GROUP);
LISTBASE_FOREACH (IDProperty *, prop, &src->data.group) {
IDProperty *other = static_cast<IDProperty *>(
BLI_findstring(&dest->data.group, prop->name, offsetof(IDProperty, name)));
IDProperty *other = IDP_GetPropertyFromGroup(dest, prop->name);
if (other && prop->type == other->type) {
switch (prop->type) {
case IDP_INT:
@@ -595,8 +601,7 @@ void IDP_SyncGroupValues(IDProperty *dest, const IDProperty *src)
IDP_SyncGroupValues(other, prop);
break;
default: {
BLI_insertlinkreplace(&dest->data.group, other, IDP_CopyProperty(prop));
IDP_FreeProperty(other);
IDP_ReplaceInGroup_ex(dest, IDP_CopyProperty(prop), other, 0);
break;
}
}
@@ -614,8 +619,7 @@ void IDP_SyncGroupTypes(IDProperty *dest, const IDProperty *src, const bool do_a
(do_arraylen && ELEM(prop_dst->type, IDP_ARRAY, IDP_IDPARRAY) &&
(prop_src->len != prop_dst->len)))
{
BLI_insertlinkreplace(&dest->data.group, prop_dst, IDP_CopyProperty(prop_src));
IDP_FreeProperty(prop_dst);
IDP_ReplaceInGroup_ex(dest, IDP_CopyProperty(prop_src), prop_dst, 0);
}
else if (prop_dst->type == IDP_GROUP) {
IDP_SyncGroupTypes(prop_dst, prop_src, do_arraylen);
@@ -633,21 +637,8 @@ void IDP_ReplaceGroupInGroup(IDProperty *dest, const IDProperty *src)
BLI_assert(src->type == IDP_GROUP);
LISTBASE_FOREACH (IDProperty *, prop, &src->data.group) {
IDProperty *loop;
for (loop = static_cast<IDProperty *>(dest->data.group.first); loop; loop = loop->next) {
if (STREQ(loop->name, prop->name)) {
BLI_insertlinkreplace(&dest->data.group, loop, IDP_CopyProperty(prop));
IDP_FreeProperty(loop);
break;
}
}
/* only add at end if not added yet */
if (loop == nullptr) {
IDProperty *copy = IDP_CopyProperty(prop);
dest->len++;
BLI_addtail(&dest->data.group, copy);
}
IDProperty *old_dest_prop = IDP_GetPropertyFromGroup(dest, prop->name);
IDP_ReplaceInGroup_ex(dest, IDP_CopyProperty(prop), old_dest_prop, 0);
}
}
@@ -660,12 +651,15 @@ void IDP_ReplaceInGroup_ex(IDProperty *group,
BLI_assert(prop_exist == IDP_GetPropertyFromGroup(group, prop->name));
if (prop_exist != nullptr) {
/* Insert the new property at the same position as the old one in the linked list. */
BLI_insertlinkreplace(&group->data.group, prop_exist, prop);
BLI_assert(group->data.children_map);
group->data.children_map->children.remove_contained(prop_exist);
group->data.children_map->children.add_new(prop);
IDP_FreeProperty_ex(prop_exist, (flag & LIB_ID_CREATE_NO_USER_REFCOUNT) == 0);
}
else {
group->len++;
BLI_addtail(&group->data.group, prop);
IDP_AddToGroup(group, prop);
}
}
@@ -709,9 +703,7 @@ void IDP_MergeGroup_ex(IDProperty *dest,
}
}
else {
IDProperty *copy = IDP_CopyProperty_ex(prop, flag);
dest->len++;
BLI_addtail(&dest->data.group, copy);
IDP_AddToGroup(dest, IDP_CopyProperty_ex(prop, flag));
}
}
}
@@ -726,25 +718,12 @@ bool IDP_AddToGroup(IDProperty *group, IDProperty *prop)
{
BLI_assert(group->type == IDP_GROUP);
if (IDP_GetPropertyFromGroup(group, prop->name) == nullptr) {
idp_group_children_map_ensure(*group);
if (group->data.children_map->children.add(prop)) {
group->len++;
BLI_addtail(&group->data.group, prop);
return true;
}
return false;
}
bool IDP_InsertToGroup(IDProperty *group, IDProperty *previous, IDProperty *pnew)
{
BLI_assert(group->type == IDP_GROUP);
if (IDP_GetPropertyFromGroup(group, pnew->name) == nullptr) {
group->len++;
BLI_insertlinkafter(&group->data.group, previous, pnew);
return true;
}
return false;
}
@@ -755,6 +734,8 @@ void IDP_RemoveFromGroup(IDProperty *group, IDProperty *prop)
group->len--;
BLI_remlink(&group->data.group, prop);
BLI_assert(group->data.children_map);
group->data.children_map->children.remove_contained(prop);
}
void IDP_FreeFromGroup(IDProperty *group, IDProperty *prop)
@@ -766,14 +747,14 @@ void IDP_FreeFromGroup(IDProperty *group, IDProperty *prop)
IDProperty *IDP_GetPropertyFromGroup(const IDProperty *prop, const blender::StringRef name)
{
BLI_assert(prop->type == IDP_GROUP);
return BLI_listbase_find<IDProperty>(prop->data.group,
[&](const IDProperty &elem) { return elem.name == name; });
}
IDProperty *IDP_GetPropertyFromGroup(const IDProperty *prop, const char *name)
{
BLI_assert(prop->type == IDP_GROUP);
return (IDProperty *)BLI_findstring(&prop->data.group, name, offsetof(IDProperty, name));
if (prop->len == 0) {
BLI_assert(prop->data.children_map == nullptr || prop->data.children_map->children.is_empty());
return nullptr;
}
/* If there is at least one item, the map is expected to exist. */
BLI_assert(prop->data.children_map);
BLI_assert(prop->data.children_map->children.size() == prop->len);
return prop->data.children_map->children.lookup_key_default_as(name, nullptr);
}
IDProperty *IDP_GetPropertyTypeFromGroup(const IDProperty *prop,
@@ -792,6 +773,7 @@ static void IDP_FreeGroup(IDProperty *prop, const bool do_id_user)
{
BLI_assert(prop->type == IDP_GROUP);
MEM_SAFE_DELETE(prop->data.children_map);
LISTBASE_FOREACH (IDProperty *, loop, &prop->data.group) {
IDP_FreePropertyContent_ex(loop, do_id_user);
}
@@ -1621,12 +1603,20 @@ static void IDP_DirectLinkString(IDProperty *prop, BlendDataReader *reader)
static void IDP_DirectLinkGroup(IDProperty *prop, BlendDataReader *reader)
{
ListBase *lb = &prop->data.group;
prop->data.children_map = nullptr;
BLO_read_struct_list(reader, IDProperty, lb);
if (!BLI_listbase_is_empty(&prop->data.group)) {
idp_group_children_map_ensure(*prop);
}
/* Link child id properties now. */
LISTBASE_FOREACH (IDProperty *, loop, &prop->data.group) {
IDP_DirectLinkProperty(loop, reader);
if (!prop->data.children_map->children.add(loop)) {
CLOG_WARN(&LOG, "duplicate ID property '%s' in group", loop->name);
}
}
}

96
source/blender/blenkernel/intern/idprop_test.cc Normal file
View File

@@ -0,0 +1,96 @@
/* SPDX-FileCopyrightText: 2025 Blender Authors
*
* SPDX-License-Identifier: GPL-2.0-or-later */
#include "BKE_idprop.hh"
#include "testing/testing.h"
namespace blender::bke::tests {
TEST(idproperties, CreateGroup)
{
IDProperty *prop = idprop::create_group("test").release();
IDP_FreeProperty(prop);
}
TEST(idproperties, AddToGroup)
{
IDProperty *group = idprop::create_group("test").release();
EXPECT_EQ(IDP_GetPropertyFromGroup(group, "a"), nullptr);
EXPECT_TRUE(IDP_AddToGroup(group, idprop::create("a", 3.0f).release()));
EXPECT_TRUE(IDP_AddToGroup(group, idprop::create("b", 5).release()));
EXPECT_EQ(IDP_Float(IDP_GetPropertyFromGroup(group, "a")), 3.0f);
EXPECT_EQ(IDP_Int(IDP_GetPropertyFromGroup(group, "b")), 5);
IDProperty *duplicate_prop = idprop::create("a", 10).release();
EXPECT_FALSE(IDP_AddToGroup(group, duplicate_prop));
EXPECT_EQ(IDP_Float(IDP_GetPropertyFromGroup(group, "a")), 3.0f);
EXPECT_EQ(IDP_GetPropertyFromGroup(group, "c"), nullptr);
IDP_FreeProperty(duplicate_prop);
IDP_FreeProperty(group);
}
TEST(idproperties, ReplaceInGroup)
{
IDProperty *group = idprop::create_group("test").release();
EXPECT_TRUE(IDP_AddToGroup(group, idprop::create("a", 3.0f).release()));
EXPECT_EQ(IDP_Float(IDP_GetPropertyFromGroup(group, "a")), 3.0f);
IDP_ReplaceInGroup(group, idprop::create("a", 5.0f).release());
EXPECT_EQ(IDP_Float(IDP_GetPropertyFromGroup(group, "a")), 5.0f);
IDP_ReplaceInGroup(group, idprop::create("b", 5).release());
EXPECT_EQ(IDP_Int(IDP_GetPropertyFromGroup(group, "b")), 5);
IDP_FreeProperty(group);
}
TEST(idproperties, RemoveFromGroup)
{
IDProperty *group = idprop::create_group("test").release();
EXPECT_EQ(IDP_GetPropertyFromGroup(group, "a"), nullptr);
IDProperty *prop_a = idprop::create("a", 3.0f).release();
EXPECT_TRUE(IDP_AddToGroup(group, prop_a));
EXPECT_EQ(IDP_Float(IDP_GetPropertyFromGroup(group, "a")), 3.0f);
IDP_RemoveFromGroup(group, prop_a);
EXPECT_EQ(IDP_GetPropertyFromGroup(group, "a"), nullptr);
IDP_FreeProperty(prop_a);
IDP_FreeProperty(group);
}
TEST(idproperties, ReplaceGroupInGroup)
{
IDProperty *group1 = idprop::create_group("test").release();
IDP_AddToGroup(group1, idprop::create("a", 1).release());
IDP_AddToGroup(group1, idprop::create("b", 2).release());
IDProperty *group2 = idprop::create_group("test2").release();
IDP_AddToGroup(group2, idprop::create("b", 3).release());
IDP_AddToGroup(group2, idprop::create("c", 4).release());
IDP_ReplaceGroupInGroup(group1, group2);
EXPECT_EQ(IDP_Int(IDP_GetPropertyFromGroup(group1, "a")), 1);
EXPECT_EQ(IDP_Int(IDP_GetPropertyFromGroup(group1, "b")), 3);
EXPECT_EQ(IDP_Int(IDP_GetPropertyFromGroup(group1, "c")), 4);
IDP_FreeProperty(group1);
IDP_FreeProperty(group2);
}
TEST(idproperties, SyncGroupValues)
{
IDProperty *group1 = idprop::create_group("test").release();
IDProperty *group2 = idprop::create_group("test").release();
IDP_AddToGroup(group1, idprop::create("a", 1).release());
IDP_AddToGroup(group1, idprop::create("b", 2).release());
IDP_AddToGroup(group1, idprop::create("x", 2).release());
IDP_AddToGroup(group2, idprop::create("a", 3).release());
IDP_AddToGroup(group2, idprop::create("c", 4).release());
IDP_AddToGroup(group2, idprop::create("x", "value").release());
IDP_SyncGroupValues(group1, group2);
EXPECT_EQ(IDP_Int(IDP_GetPropertyFromGroup(group1, "a")), 3);
EXPECT_EQ(IDP_Int(IDP_GetPropertyFromGroup(group1, "b")), 2);
EXPECT_EQ(IDP_Int(IDP_GetPropertyFromGroup(group1, "x")), 2);
EXPECT_EQ(IDP_GetPropertyFromGroup(group1, "c"), nullptr);
IDP_FreeProperty(group1);
IDP_FreeProperty(group2);
}
} // namespace blender::bke::tests

3
source/blender/io/fbx/importer/fbx_import_util.cc
View File

@@ -265,8 +265,7 @@ void read_custom_properties(const ufbx_props &props, ID &id, bool enums_as_strin
static IDProperty *pchan_EnsureProperties(bPoseChannel &pchan)
{
if (pchan.prop == nullptr) {
pchan.prop = MEM_callocN<IDProperty>("IDProperty");
pchan.prop->type = IDP_GROUP;
pchan.prop = bke::idprop::create_group("").release();
}
return pchan.prop;
}

10
source/blender/makesdna/DNA_ID.h
View File

@@ -23,14 +23,19 @@
namespace blender::bke {
struct PreviewImageRuntime;
}
namespace blender::bke::idprop {
struct IDPropertyGroupChildrenSet;
}
namespace blender::bke::library {
struct LibraryRuntime;
}
using PreviewImageRuntimeHandle = blender::bke::PreviewImageRuntime;
using LibraryRuntimeHandle = blender::bke::library::LibraryRuntime;
using IDPropertyGroupChildrenSet = blender::bke::idprop::IDPropertyGroupChildrenSet;
#else
typedef struct PreviewImageRuntimeHandle PreviewImageRuntimeHandle;
typedef struct LibraryRuntimeHandle LibraryRuntimeHandle;
typedef struct IDPropertyGroupChildrenSet IDPropertyGroupChildrenSet;
#endif
#ifdef __cplusplus
@@ -136,6 +141,11 @@ typedef struct IDPropertyUIDataID {
typedef struct IDPropertyData {
void *pointer;
ListBase group;
/**
* Allows constant time lookup by name of the children in this group. This may be null if the
* group is empty. The order may not be exactly the same as in #group.
*/
IDPropertyGroupChildrenSet *children_map;
/** NOTE: a `double` is written into two 32bit integers. */
int val, val2;
} IDPropertyData;