Skip to content

Allow CTE on basic type/math functions #10842

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 10 commits into from
Apr 24, 2023
Merged

Allow CTE on basic type/math functions #10842

merged 10 commits into from
Apr 24, 2023

Conversation

mvorisek
Copy link
Contributor

as requested in #10771 (comment)

once this PR is merged, we can discuss the remaining functions

@mvorisek
Copy link
Contributor Author

@TimWolla @nielsdos can you please review?

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.

Thanks!

This reverts commit 069fa1a.
@nielsdos
Copy link
Member

I went through the PR again. I think the functions on which you added CTE are safe.
I only have 2 additional remarks:

  1. For some functions, like number_format() and variadic ones, the benefit might be only very small. That's because the optimizer limits the amount of arguments processed in CTE calls to 3. This is done for simplicity, and for compile-time performance (a lot of works happens for CTE stuff, so it makes sense to limit this).
  2. I see there's some discussion about the CTE & refcount attribute ordering. IIRC someone told you to reorder this and now it is requested that the reordering is undone. I don't know what the best way forward is here. I'd just look at the most commonly used ordering and use that in new code, while keeping the old code unchanged to avoid extra code churn.

@mvorisek mvorisek requested a review from iluuu1994 March 23, 2023 20:22
@mvorisek
Copy link
Contributor Author

For some functions, like number_format() and variadic ones, the benefit might be only very small. That's because the optimizer limits the amount of arguments processed in CTE calls to 3. This is done for simplicity, and for compile-time performance (a lot of works happens for CTE stuff, so it makes sense to limit this).

As long as a function is CTEable, I would say there should be no arbitrary limit of arguments.

I see there's some discussion about the CTE & refcount attribute ordering.

Yes, someone wants it, someone not. I would say 6 total changes across the whole php-src are worth vs. the total "preferred" order.

@mvorisek
Copy link
Contributor Author

Let's please merge this PR if there is no feedback left so I can finish finish another PR on top of it. Thank you.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

It seems that first compile-time-eval followed by refcount is the most common form. So therefore I think making this consistent in this PR is fine.
The functions marked as CTE were determined to be correctly marked.

function strval(mixed $value): string {}

/**
* @compile-time-eval
*/
function is_null(mixed $value): bool {}

function is_resource(mixed $value): bool {}
Copy link
Member

Choose a reason for hiding this comment

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

Can is_resource cause issues when called at compile time? Otherwise, it could still eliminate calls when elements are not resources.

Copy link
Contributor Author

@mvorisek mvorisek Apr 24, 2023

Choose a reason for hiding this comment

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

Correct me if I am wrong, but can resource be even observed on CT?

Copy link
Member

Choose a reason for hiding this comment

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

No, but it could eliminate calls like is_resource(42).

Copy link
Contributor Author

@mvorisek mvorisek Apr 24, 2023

Choose a reason for hiding this comment

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

I added is_resource, tests pass, the 1 ASAN failure is unrelated.

@mvorisek mvorisek requested a review from kocsismate as a code owner April 24, 2023 16:08
@nielsdos nielsdos merged commit 1209f59 into php:master Apr 24, 2023
@mvorisek mvorisek deleted the math_cte branch April 24, 2023 21:55
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