Skip to content
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

Implement acyclic object tracking #17130

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Implement acyclic object tracking #17130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f6e49bb
Implement acyclic object tracking
iluuu1994 Dec 12, 2024
969a472
Automatically handle ZEND_ACC_MAY_BE_CYCLIC for extensions
iluuu1994 Dec 13, 2024
4eddbc5
Fix leak in Zend/tests/bug64720.phpt
iluuu1994 Dec 13, 2024
786c14a
Fixes and improvements
iluuu1994 Dec 16, 2024
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
6 changes: 6 additions & 0 deletions Zend/tests/gc/gc_045.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ class GlobalData

class Value
{
/* Force object to be added to GC, even though it is acyclic. */
public $dummy;

public function __destruct()
{
new Bar();
Expand All @@ -19,6 +22,9 @@ class Value

class Bar
{
/* Force object to be added to GC, even though it is acyclic. */
public $dummy;

public function __construct()
{
GlobalData::$bar = $this;
Expand Down
3 changes: 3 additions & 0 deletions Zend/tests/weakrefs/gh10043-008.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ Self-referencing map entry GC - 008

class Canary extends stdClass
{
/* Force object to be added to GC, even though it is acyclic. */
public $dummy;

public function __construct(public string $name)
{
}
Expand Down
47 changes: 47 additions & 0 deletions Zend/zend_API.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1770,6 +1770,7 @@ ZEND_API void object_properties_load(zend_object *object, HashTable *properties)
ZSTR_VAL(object->ce->name), property_info != ZEND_WRONG_PROPERTY_INFO ? zend_get_unmangled_property_name(key): "");
}

GC_TYPE_INFO(object) &= ~(GC_NOT_COLLECTABLE << GC_FLAGS_SHIFT);
prop = zend_hash_update(zend_std_get_properties_ex(object), key, prop);
zval_add_ref(prop);
}
Expand All @@ -1782,6 +1783,7 @@ ZEND_API void object_properties_load(zend_object *object, HashTable *properties)
ZSTR_VAL(object->ce->name), h);
}

GC_TYPE_INFO(object) &= ~(GC_NOT_COLLECTABLE << GC_FLAGS_SHIFT);
prop = zend_hash_index_update(zend_std_get_properties_ex(object), h, prop);
zval_add_ref(prop);
}
Expand Down Expand Up @@ -2390,6 +2392,8 @@ ZEND_API zend_result zend_startup_module_ex(zend_module_entry *module) /* {{{ */
}
module->module_started = 1;

uint32_t prev_class_count = zend_hash_num_elements(CG(class_table));

/* Check module dependencies */
if (module->deps) {
const zend_module_dep *dep = module->deps;
Expand Down Expand Up @@ -2434,6 +2438,22 @@ ZEND_API zend_result zend_startup_module_ex(zend_module_entry *module) /* {{{ */
}
EG(current_module) = NULL;
}

/* Mark classes with custom get_gc handler as potentially cyclic, even if
* their properties don't indicate so. */
Comment on lines +2442 to +2443
Copy link
Member

Choose a reason for hiding this comment

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

We should also mark classes with custom get_properties handler as potentially cyclic, as this is called by zend_std_get_gc.

Otherwise this looks right to me, as objects with std get_gc and get_properties can not expose anything other than standard properties to the GC.

Lazy objects need to take care of updating GC_NOT_COLLECTABLE: Remove GC_NOT_COLLECTABLE in zend_object_make_lazy() when the initializer is cyclic or may have a ref to the object itself, and add it again in zend_lazy_object_init().

if (prev_class_count != zend_hash_num_elements(CG(class_table))) {
Bucket *p;
ZEND_HASH_MAP_FOREACH_BUCKET_FROM(CG(class_table), p, prev_class_count) {
zend_class_entry *ce = Z_PTR(p->val);
if ((ce->ce_flags & ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES)
|| ce->create_object
|| ce->default_object_handlers->get_gc != zend_std_get_gc
|| ce->default_object_handlers->get_properties != zend_std_get_properties) {
ce->ce_flags |= ZEND_ACC_MAY_BE_CYCLIC;
}
} ZEND_HASH_FOREACH_END();
}

return SUCCESS;
}
/* }}} */
Expand Down Expand Up @@ -4494,6 +4514,27 @@ static zend_always_inline bool is_persistent_class(zend_class_entry *ce) {
&& ce->info.internal.module->type == MODULE_PERSISTENT;
}

static bool zend_type_may_be_cyclic(zend_type type)
{
if (!ZEND_TYPE_IS_SET(type)) {
return true;
}

if (!ZEND_TYPE_IS_COMPLEX(type)) {
return ZEND_TYPE_PURE_MASK(type) & (MAY_BE_OBJECT|MAY_BE_ARRAY);
} else if (ZEND_TYPE_IS_UNION(type)) {
zend_type *list_type;
ZEND_TYPE_LIST_FOREACH(ZEND_TYPE_LIST(type), list_type) {
if (zend_type_may_be_cyclic(*list_type)) {
return true;
}
} ZEND_TYPE_LIST_FOREACH_END();
return false;
}

return true;
}

ZEND_API zend_property_info *zend_declare_typed_property(zend_class_entry *ce, zend_string *name, zval *property, int access_type, zend_string *doc_comment, zend_type type) /* {{{ */
{
zend_property_info *property_info, *property_info_ptr;
Expand All @@ -4506,6 +4547,12 @@ ZEND_API zend_property_info *zend_declare_typed_property(zend_class_entry *ce, z
}
}

if (!(access_type & ZEND_ACC_STATIC)
&& !(ce->ce_flags & ZEND_ACC_MAY_BE_CYCLIC)
&& zend_type_may_be_cyclic(type)) {
ce->ce_flags |= ZEND_ACC_MAY_BE_CYCLIC;
}

if (ce->type == ZEND_INTERNAL_CLASS) {
property_info = pemalloc(sizeof(zend_property_info), 1);
} else {
Expand Down
2 changes: 2 additions & 0 deletions Zend/zend_closures.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,8 @@ void zend_register_closure_ce(void) /* {{{ */
zend_ce_closure = register_class_Closure();
zend_ce_closure->create_object = zend_closure_new;
zend_ce_closure->default_object_handlers = &closure_handlers;
/* FIXME: Potentially infer ZEND_ACC_MAY_BE_CYCLIC during construction of
* closure? static closures not binding by references can't be cyclic. */
Comment on lines +708 to +709
Copy link
Member

Choose a reason for hiding this comment

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

👍 this looks worth it

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm afraid my comment was incorrect.

class Test {
    public \Closure $c;
}

$test = new Test();
$c = static function () use ($test) {}; // acyclic
$test->c = $c; // now it's cyclic
unset($test);
gc_collect_cycles();
unset($c); // leaks

This is only ok if $test itself is acyclic. As mentioned previously, this check would currently be unsound because the object may become cyclic at a later point in time by adding dynamic properties. However, once we're on PHP 9, it should be ok!

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth mentioning: readonly classes don't permit dynamic properties. So readonly classes could benefit from this optimization immediately without waiting for PHP 9.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, your test case doesn't show any dynamic property usage @iluuu1994, wouldn't that still be an issue in non-readonly cases even in PHP 9.0?

Copy link
Member Author

@iluuu1994 iluuu1994 Dec 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Might be worth mentioning: readonly classes don't permit dynamic properties. So readonly classes could benefit from this optimization immediately without waiting for PHP 9.0.

I was referring to:

This is only ok if $test itself is acyclic. As mentioned previously, this check would currently be unsound

I.e. it's safe to assume Closure is acyclic only if it captures values that aren't themselves acyclic. If Test may point to cyclic values, as Closure itself, then it won't be marked as acyclic. But today, even if Test were inferrably acyclic, this is not guaranteed to remain this way due to dynamic properties. Hence, we need to be more careful about relations between objects.

Actually, your test case doesn't show any dynamic property usage @iluuu1994, wouldn't that still be an issue in non-readonly cases even in PHP 9.0?

Yes, but it's not a priority since it's a rare case and unlikely to make a big difference.


memcpy(&closure_handlers, &std_object_handlers, sizeof(zend_object_handlers));
closure_handlers.free_obj = zend_closure_free_storage;
Expand Down
5 changes: 4 additions & 1 deletion Zend/zend_compile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ typedef struct _zend_oparray_context {
#define ZEND_ACC_PROTECTED_SET (1 << 11) /* | | X | */
#define ZEND_ACC_PRIVATE_SET (1 << 12) /* | | X | */
/* | | | */
/* Class Flags (unused: 30,31) | | | */
/* Class Flags (unused: 31) | | | */
/* =========== | | | */
/* | | | */
/* Special class types | | | */
Expand Down Expand Up @@ -333,6 +333,9 @@ typedef struct _zend_oparray_context {
/* Class cannot be serialized or unserialized | | | */
#define ZEND_ACC_NOT_SERIALIZABLE (1 << 29) /* X | | | */
/* | | | */
/* Object may be the root of a cycle | | | */
#define ZEND_ACC_MAY_BE_CYCLIC (1 << 30) /* X | | | */
/* | | | */
/* Function Flags (unused: 29-30) | | | */
/* ============== | | | */
/* | | | */
Expand Down
Loading
Loading