Re: [PATCH 1/3] qom/object: merge double hash table traversal in object_property_del_child
On 30/4/26 11:24, Daniel P. Berrangé wrote:
On Thu, Apr 30, 2026 at 07:07:03AM +1000, Richard Henderson wrote:
On 4/29/26 16:20, Bin Guo wrote:
object_property_del_child() previously performed two full iterations
over obj->properties: the first to find the matching child property and
call its release callback, the second to find the same property again
and remove it from the hash table.
Merge the two loops into one: when the matching property is found, call
the release callback and immediately remove the entry via
g_hash_table_iter_remove(), then break. This halves the number of hash
table operations in the common case and avoids the redundant second scan.
Signed-off-by: Bin Guo
---
qom/object.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index f981e27044..9c0e8dfd02 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -629,13 +629,6 @@ static void object_property_del_child(Object *obj, Object
*child)
prop->release(obj, prop->name, prop->opaque);
prop->release = NULL;
}
-break;
-}
-}
-g_hash_table_iter_init(&iter, obj->properties);
-while (g_hash_table_iter_next(&iter, &key, &value)) {
-prop = value;
-if (object_property_is_child(prop) && prop->opaque == child) {
g_hash_table_iter_remove(&iter);
break;
}
Fixes: b604a854e84 ("qom: Replace object property list with GHashTable")
On the contrary, this proposed patch breaks that commit.
The original proposal for that old commit had a single iteration,
but it was discovered that this broke cleanup for certain devices,
as prop->release can have side-effects that trigger assertions in
g_hash_table_iter_remove. This requires a two phase iteration
to be safe.
Worth a comment in the code :)
A test for that problem was added in 8c4d156c187c84b574d287bd4b9ddf9a6975de7c
and this new patch proposal duly breaks the check-qom-proplist test.
With regards,
Daniel
Re: [PATCH 1/3] qom/object: merge double hash table traversal in object_property_del_child
On Thu, Apr 30, 2026 at 07:07:03AM +1000, Richard Henderson wrote:
> On 4/29/26 16:20, Bin Guo wrote:
> > object_property_del_child() previously performed two full iterations
> > over obj->properties: the first to find the matching child property and
> > call its release callback, the second to find the same property again
> > and remove it from the hash table.
> >
> > Merge the two loops into one: when the matching property is found, call
> > the release callback and immediately remove the entry via
> > g_hash_table_iter_remove(), then break. This halves the number of hash
> > table operations in the common case and avoids the redundant second scan.
> >
> > Signed-off-by: Bin Guo
> > ---
> > qom/object.c | 7 ---
> > 1 file changed, 7 deletions(-)
> >
> > diff --git a/qom/object.c b/qom/object.c
> > index f981e27044..9c0e8dfd02 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -629,13 +629,6 @@ static void object_property_del_child(Object *obj,
> > Object *child)
> > prop->release(obj, prop->name, prop->opaque);
> > prop->release = NULL;
> > }
> > -break;
> > -}
> > -}
> > -g_hash_table_iter_init(&iter, obj->properties);
> > -while (g_hash_table_iter_next(&iter, &key, &value)) {
> > -prop = value;
> > -if (object_property_is_child(prop) && prop->opaque == child) {
> > g_hash_table_iter_remove(&iter);
> > break;
> > }
>
> Fixes: b604a854e84 ("qom: Replace object property list with GHashTable")
On the contrary, this proposed patch breaks that commit.
The original proposal for that old commit had a single iteration,
but it was discovered that this broke cleanup for certain devices,
as prop->release can have side-effects that trigger assertions in
g_hash_table_iter_remove. This requires a two phase iteration
to be safe.
A test for that problem was added in 8c4d156c187c84b574d287bd4b9ddf9a6975de7c
and this new patch proposal duly breaks the check-qom-proplist test.
With regards,
Daniel
Re: [PATCH 1/3] qom/object: merge double hash table traversal in object_property_del_child
On 4/29/26 16:20, Bin Guo wrote:
object_property_del_child() previously performed two full iterations
over obj->properties: the first to find the matching child property and
call its release callback, the second to find the same property again
and remove it from the hash table.
Merge the two loops into one: when the matching property is found, call
the release callback and immediately remove the entry via
g_hash_table_iter_remove(), then break. This halves the number of hash
table operations in the common case and avoids the redundant second scan.
Signed-off-by: Bin Guo
---
qom/object.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index f981e27044..9c0e8dfd02 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -629,13 +629,6 @@ static void object_property_del_child(Object *obj, Object
*child)
prop->release(obj, prop->name, prop->opaque);
prop->release = NULL;
}
-break;
-}
-}
-g_hash_table_iter_init(&iter, obj->properties);
-while (g_hash_table_iter_next(&iter, &key, &value)) {
-prop = value;
-if (object_property_is_child(prop) && prop->opaque == child) {
g_hash_table_iter_remove(&iter);
break;
}
Fixes: b604a854e84 ("qom: Replace object property list with GHashTable")
Reviewed-by: Richard Henderson
r~
[PATCH 1/3] qom/object: merge double hash table traversal in object_property_del_child
object_property_del_child() previously performed two full iterations
over obj->properties: the first to find the matching child property and
call its release callback, the second to find the same property again
and remove it from the hash table.
Merge the two loops into one: when the matching property is found, call
the release callback and immediately remove the entry via
g_hash_table_iter_remove(), then break. This halves the number of hash
table operations in the common case and avoids the redundant second scan.
Signed-off-by: Bin Guo
---
qom/object.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index f981e27044..9c0e8dfd02 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -629,13 +629,6 @@ static void object_property_del_child(Object *obj, Object
*child)
prop->release(obj, prop->name, prop->opaque);
prop->release = NULL;
}
-break;
-}
-}
-g_hash_table_iter_init(&iter, obj->properties);
-while (g_hash_table_iter_next(&iter, &key, &value)) {
-prop = value;
-if (object_property_is_child(prop) && prop->opaque == child) {
g_hash_table_iter_remove(&iter);
break;
}
--
2.50.1 (Apple Git-155)
