Skip to content

Zend: use zend_object* for zend_closure this_ptr field - #23246

Merged
Girgias merged 2 commits into
php:masterfrom
Girgias:2026-08-closure-this-zend_object
Aug 14, 2026
Merged

Zend: use zend_object* for zend_closure this_ptr field#23246
Girgias merged 2 commits into
php:masterfrom
Girgias:2026-08-closure-this-zend_object

Conversation

@Girgias

@Girgias Girgias commented Aug 13, 2026

Copy link
Copy Markdown
Member

The main motivation was to convert Z_PARAM_OBJECT{_OR_NULL} to Z_PARAM_OBJ{_OR_NULL}, but I do think working with a zend_object* makes the code more legible and there are no question if this zval can ever be a different type.

Not fully certain about the JIT changes, as maybe the double load is inefficient?

@Girgias
Girgias force-pushed the 2026-08-closure-this-zend_object branch 2 times, most recently from 3bad973 to 9322da2 Compare August 13, 2026 18:00
@Girgias
Girgias marked this pull request as ready for review August 13, 2026 18:51
@Girgias
Girgias requested a review from arnaud-lb August 13, 2026 18:51

@DanielEScherzer DanielEScherzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

okay for ext/reflection, two suggestions elsewhere

Comment thread ext/opcache/jit/zend_jit_ir.c Outdated
Comment thread Zend/zend_closures.c Outdated
Comment on lines +974 to +977
zval instance;
ZVAL_OBJ(&instance, Z_OBJ(call->This));

zend_create_fake_closure(return_value, mptr, mptr->common.scope, Z_OBJCE(instance), &instance);
zend_create_fake_closure(return_value, mptr, mptr->common.scope, Z_OBJCE(instance), Z_OBJ(instance));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems like this is doing an unnecessary setup of instance only to extract the object that we already have

Suggested change
zval instance;
ZVAL_OBJ(&instance, Z_OBJ(call->This));
zend_create_fake_closure(return_value, mptr, mptr->common.scope, Z_OBJCE(instance), &instance);
zend_create_fake_closure(return_value, mptr, mptr->common.scope, Z_OBJCE(instance), Z_OBJ(instance));
zend_create_fake_closure(return_value, mptr, mptr->common.scope, Z_OBJCE(call->This), Z_OBJ(call->This));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought so too, but in reality it is needed to preserve the "this" of the current frame (I think) as call->This is pointing to the VM stack.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

okay, then can I suggest adding a comment to explain that so that others don't try to do the simplification that looked obvious to me?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Okay, turns out it no longer gives test failures so I'll drop it.

@arnaud-lb

Copy link
Copy Markdown
Member

Not fully certain about the JIT changes, as maybe the double load is inefficient?

These look right. The double load appears to be optimized out, but for clarity it would be worth it to de-duplicate. Also it looks like that the line object_or_called_scope = ir_LOAD_A(ir_ADD_OFFSET(func_ref, offsetof(zend_closure, called_scope))); could be moved in a ir_FALSE(if_cond), but that's not really related to this PR.

@arnaud-lb arnaud-lb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me otherwise

Instead of a zval*, this is in preparation for converting the this_ptr field of zend_closure to zend_object*
@Girgias
Girgias force-pushed the 2026-08-closure-this-zend_object branch from 9322da2 to 6fa9ff0 Compare August 14, 2026 12:51
@Girgias
Girgias merged commit 7a5e452 into php:master Aug 14, 2026
18 checks passed
@Girgias
Girgias deleted the 2026-08-closure-this-zend_object branch August 14, 2026 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants