Skip to content

Fix RC func info of str_split#9016

Merged
iluuu1994 merged 1 commit into
php:masterfrom
iluuu1994:fix-str_split_rc-func-info
Jul 15, 2022
Merged

Fix RC func info of str_split#9016
iluuu1994 merged 1 commit into
php:masterfrom
iluuu1994:fix-str_split_rc-func-info

Conversation

@iluuu1994

Copy link
Copy Markdown
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

mvorisek commented Jul 15, 2022

Copy link
Copy Markdown
Contributor

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

@cmb69 cmb69 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.

Thank you!

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

Copy link
Copy Markdown
Contributor

@iluuu1994

iluuu1994 commented Jul 15, 2022

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
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

cmb69 commented Jul 15, 2022

Copy link
Copy Markdown
Member

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