Skip to content

Fix RC func info of str_split #9016

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
Jul 15, 2022

Conversation

iluuu1994
Copy link
Member

See test failure in nightly.

Introduced in GH-8945

With RETURN_EMPTY_ARRAY this function can now return an interned array which
has refcount 2.

Introduced in phpGH-8945

With RETURN_EMPTY_ARRAY this function can now return an interned array which
has refcount 2.
@mvorisek
Copy link
Contributor

mvorisek commented Jul 15, 2022

Has F1 any better performance or when it is helpful? Also, why did GH nightly not fail?

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thank you!

@iluuu1994 iluuu1994 merged commit 63912b5 into php:master Jul 15, 2022
@mvorisek
Copy link
Contributor

@iluuu1994
Copy link
Member Author

iluuu1994 commented Jul 15, 2022

@mvorisek Nightly still isn't fully migrated. Look at the job configuration, you'll see that they are different. Refcount information is used in the optimizer, so potentially yes. But the difference is likely negligible, and an empty array will be faster than the previous array containing an empty string as an empty array doesn't cause any allocations.

@mvorisek
Copy link
Contributor

That is my point. When "empty array doesn't cause any allocations", what is the purpose of interning such value then? Maybe there are more places where "better" F1 cannot be returned just because of this.

@cmb69
Copy link
Member

cmb69 commented Jul 15, 2022

An empty array would require an allocation; this is why we use a statically allocated empty array. To avoid that this ever gets freed, it has a refcount of 2.

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