Skip to content

Improve the optimizer's check if a function is a prototype or not #10467

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.

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 1 commit into from
Feb 20, 2023

Conversation

nielsdos
Copy link
Member

Currently, a function is considered a prototype if the function is not marked as final. However, a class marked as final or an anonymous class also make it impossible for a function to be overridden. Therefore, we know in these 2 cases too that the function is not a prototype. This allows the type inference algorithm to determine some types more precisely, and can allow for more optimizations of the instructions. Additionally, place some computation of the flags in their respective blocks as a micro-optimization.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

This looks good to me, just want a second set of eyes to be absolutely sure as I don't touch the Optimizer/OpCache much.

@nielsdos
Copy link
Member Author

nielsdos commented Feb 16, 2023

I pushed a minor optimization: combined (fbc->common.scope->ce_flags & ZEND_ACC_FINAL) == 0 && (fbc->common.scope->ce_flags & ZEND_ACC_ANON_CLASS) == 0; into one check: (fbc->common.scope->ce_flags & (ZEND_ACC_FINAL | ZEND_ACC_ANON_CLASS)) == 0 because it generates better assembly (one less test and branch instruction). See review comment below: this no longer applies.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Makes sense to me too. Thank you @nielsdos!

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

The anonymous class needs to be removed from the test, but apart from that LGTM

@nielsdos
Copy link
Member Author

The anonymous class needs to be removed from the test, but apart from that LGTM

Ugh, can't believe I missed that... Should be fixed now.

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

Looks good.

What was wrong with anonymous classes?

Comment on lines 903 to 905
bool can_be_overridden = (fbc->common.fn_flags & ZEND_ACC_FINAL) == 0 &&
(fbc->common.scope->ce_flags & ZEND_ACC_FINAL) == 0;
if (can_be_overridden) {
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't make sense to introduce can_be_overridden variable. The condition may be used directly in the following if statement.

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'll change that thanks.
About anonymous classes, see #10467 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I'll change that thanks. About anonymous classes, see #10467 (comment)

Ugh, this is like overriding constants values through reflection API in Java :)
Anyway, your patch is right now.

Currently, a function is considered a prototype if the function is not
marked as final. However, a class marked as final also make it
impossible for a function to be overridden. Therefore, we know in this
case too that the function is not a prototype.
This allows the type inference algorithm to determine some types more
precisely, and can allow for more optimizations of the instructions.
Additionally, place some computation of the flags in their respective
blocks as a micro-optimization.

Note: anonymous classes *can* be extended (see test
Zend/tests/anon/011.phpt). Therefore we don't optimize this case.
@nielsdos
Copy link
Member Author

Pushed the change to get rid of the variable. Thanks.

@dstogov dstogov merged commit 2e78c08 into php:master Feb 20, 2023
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.

5 participants