Skip to content

zend_weakrefs: Remove layer of indirection in zend_weakrefs_hash_clean() #19580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 26, 2025
Merged

zend_weakrefs: Remove layer of indirection in zend_weakrefs_hash_clean() #19580

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4884fec
zend_weakrefs: Remove layer of indirection in `zend_weakrefs_hash_cle…
TimWolla Aug 25, 2025
08de135
zend_weakrefs: Include `zend_weakmap_free_obj()` optimization in `zen…
TimWolla Aug 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 17 additions & 15 deletions Zend/zend_weakrefs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,8 @@ static void zend_weakref_unregister(zend_object *object, void *payload, bool wea
if (weakref_free) {
zend_weakref_unref_single(ptr, tag, object);
} else {
/* The optimization of skipping unref is only used in the destructor of WeakMap */
ZEND_ASSERT(ZEND_WEAKREF_GET_TAG(payload) == ZEND_WEAKREF_TAG_MAP);
/* The optimization of skipping unref is used for zend_weakrefs_hash_clean_ex() */
ZEND_ASSERT(ZEND_WEAKREF_GET_TAG(payload) == ZEND_WEAKREF_TAG_MAP || ZEND_WEAKREF_GET_TAG(payload) == ZEND_WEAKREF_TAG_BARE_HT);
}
return;
}
Expand All @@ -163,8 +163,8 @@ static void zend_weakref_unregister(zend_object *object, void *payload, bool wea
zend_weakref_unref_single(
ZEND_WEAKREF_GET_PTR(payload), ZEND_WEAKREF_GET_TAG(payload), object);
} else {
/* The optimization of skipping unref is only used in the destructor of WeakMap */
ZEND_ASSERT(ZEND_WEAKREF_GET_TAG(payload) == ZEND_WEAKREF_TAG_MAP);
/* The optimization of skipping unref is used for zend_weakrefs_hash_clean_ex() */
ZEND_ASSERT(ZEND_WEAKREF_GET_TAG(payload) == ZEND_WEAKREF_TAG_MAP || ZEND_WEAKREF_GET_TAG(payload) == ZEND_WEAKREF_TAG_BARE_HT);
}
}

Expand All @@ -187,11 +187,20 @@ ZEND_API zend_result zend_weakrefs_hash_del(HashTable *ht, zend_object *key) {
return FAILURE;
}

ZEND_API void zend_weakrefs_hash_clean(HashTable *ht) {
static void zend_weakrefs_hash_clean_ex(HashTable *ht, int type) {
zend_ulong obj_key;
ZEND_HASH_FOREACH_NUM_KEY(ht, obj_key) {
zend_weakrefs_hash_del(ht, zend_weakref_key_to_object(obj_key));
ZEND_HASH_MAP_FOREACH_NUM_KEY(ht, obj_key) {
/* Optimization: Don't call zend_weakref_unref_single to free individual entries from ht when unregistering (which would do a hash table lookup, call zend_hash_index_del, and skip over any bucket collisions).
* Let freeing the corresponding values for WeakMap entries be done in zend_hash_clean, freeing objects sequentially.
* The performance difference is notable for larger WeakMaps with worse cache locality. */
zend_weakref_unregister(
zend_weakref_key_to_object(obj_key), ZEND_WEAKREF_ENCODE(ht, type), 0);
} ZEND_HASH_FOREACH_END();
zend_hash_clean(ht);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a destroy happens right after, no need to clean before. Maybe move that hash_clean to zend_weakrefs_hash_clean.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in Slack: zend_hash_destroy() is fairly cheap when already cleaned and I dislike making zend_weakrefs_hash_clean_ex() unsafe by leaving the dangling entries in the HashTable.

}

ZEND_API void zend_weakrefs_hash_clean(HashTable *ht) {
zend_weakrefs_hash_clean_ex(ht, ZEND_WEAKREF_TAG_BARE_HT);
}

void zend_weakrefs_init(void) {
Expand Down Expand Up @@ -340,14 +349,7 @@ static zend_object *zend_weakmap_create_object(zend_class_entry *ce)
static void zend_weakmap_free_obj(zend_object *object)
{
zend_weakmap *wm = zend_weakmap_from(object);
zend_ulong obj_key;
ZEND_HASH_MAP_FOREACH_NUM_KEY(&wm->ht, obj_key) {
/* Optimization: Don't call zend_weakref_unref_single to free individual entries from wm->ht when unregistering (which would do a hash table lookup, call zend_hash_index_del, and skip over any bucket collisions).
* Let freeing the corresponding values for WeakMap entries be done in zend_hash_destroy, freeing objects sequentially.
* The performance difference is notable for larger WeakMaps with worse cache locality. */
zend_weakref_unregister(
zend_weakref_key_to_object(obj_key), ZEND_WEAKREF_ENCODE(&wm->ht, ZEND_WEAKREF_TAG_MAP), 0);
} ZEND_HASH_FOREACH_END();
zend_weakrefs_hash_clean_ex(&wm->ht, ZEND_WEAKREF_TAG_MAP);
zend_hash_destroy(&wm->ht);
zend_object_std_dtor(&wm->std);
}
Expand Down