Skip to content

Allow optimizer to depend on preloaded symbols#15021

Merged
iluuu1994 merged 6 commits intophp:masterfrom
iluuu1994:preload-optimizer-improvements
Aug 2, 2024
Merged

Allow optimizer to depend on preloaded symbols#15021
iluuu1994 merged 6 commits intophp:masterfrom
iluuu1994:preload-optimizer-improvements

Conversation

@iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Jul 19, 2024

It is safe for the optimizer to rely on preloaded symbols from other files. This can occur when compiling non-preloaded files, referencing preloaded ones.

@arnaud-lb
Copy link
Member

arnaud-lb commented Jul 19, 2024

I double checked, and we can not assume that ZEND_ACC_PRELOADED symbols never change, after all.

The flag is added on all symbols compiled during preloading, including ones that are not always added to the persistent symbol tables. This example will generate bad code. When running with --repeat 2 -d opcache.preload=preload.php, required.php is initially compiled with g(): int, which is assumed to never change and the VERIFY_RETURN_TYPE op is removed. But when executed the second time we have g(): object.

However, symbols in EG(class_table) / EG(function_table) that are in buckets below EG(persistent_class_count) / EG(persistent_functions_count) are guaranteed to never change if they are linked. So we could replace the ce_flags & ZEND_ACC_PRELOADED check by a bucket check.

Edit: updated example (missing file)

@iluuu1994
Copy link
Member Author

Thanks you Arnaud. I'll look into this more then.

@dstogov
Copy link
Member

dstogov commented Jul 22, 2024

I think it makes sense to check for both - ZEND_ACC_PRELOADED flag and then for EG(persistent_class_count) / EG(persistent_functions_count).

@iluuu1994 iluuu1994 force-pushed the preload-optimizer-improvements branch 2 times, most recently from 09ecf7d to 7e6fefe Compare July 23, 2024 11:15
@iluuu1994 iluuu1994 force-pushed the preload-optimizer-improvements branch from 7e6fefe to 2c18a3c Compare July 24, 2024 11:05
Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

LGTM!

@iluuu1994 iluuu1994 merged commit 2e9cc9b into php:master Aug 2, 2024
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Aug 22, 2024
This issue was introduced in phpGH-15021. When building the call graph, we can now
see preloaded functions. However, building the call graph involves mutating the
callee, which we don't want to do for functions not coming from the script,
especially because the call graph is released later on.

Fixes phpGH-15490
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Aug 22, 2024
This issue was introduced in phpGH-15021. When building the call graph, we can now
see preloaded functions. However, building the call graph involves adding the
function to the caller list of the callee, which we don't want to do for
functions not coming from the script, given that call graphs are temporary.

Fixes phpGH-15490
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Aug 22, 2024
This issue was introduced in phpGH-15021. When building the call graph, we can now
see preloaded functions. However, building the call graph involves adding the
function to the caller list of the callee, which we don't want to do for
functions not coming from the script.

Fixes phpGH-15490
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Aug 22, 2024
This issue was introduced in phpGH-15021. When building the call graph, we can now
see preloaded functions. However, building the call graph involves adding the
function to the caller list of the callee, which we don't want to do for
functions not coming from the script.

Fixes phpGH-15490
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Aug 22, 2024
This issue was introduced in phpGH-15021. When building the call graph, we can now
see preloaded functions. However, building the call graph involves adding the
function to the caller list of the callee, which we don't want to do for
functions not coming from the script.

Fixes phpGH-15490
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Aug 22, 2024
This issue was introduced in phpGH-15021. When building the call graph, we can now
see preloaded functions. However, building the call graph involves adding the
function to the caller list of the callee, which we don't want to do for
functions not coming from the script.

Fixes phpGH-15490
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Aug 26, 2024
This issue was introduced in phpGH-15021. When building the call graph, we can now
see preloaded functions. However, building the call graph involves adding the
function to the caller list of the callee, which we don't want to do for
functions not coming from the script.

Fixes phpGH-15490
iluuu1994 added a commit that referenced this pull request Aug 26, 2024
This issue was introduced in GH-15021. When building the call graph, we can now
see preloaded functions. However, building the call graph involves adding the
function to the caller list of the callee, which we don't want to do for
functions not coming from the script.

Fixes GH-15490
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Feb 23, 2026
Since phpGH-15021 preloaded constants are propagated to compiled scripts. This is
problematic for file cache, which assumes all referenced zvals are either
persistently allocated or local to the current script. However, preloaded
constants live in shm as immutable, but not persistent.

To solve this, we'd need to duplicate propagated constants in the optimizer when
file cache is used. This is error prone given it needs to happen in many places.
It's debatable whether constant propagation is even correct in this case, as
running the preloaded script on a restart isn't guaranteed to produce the same
result.

Hence, avoid the issue for now by just not relying on preloaded symbols when
file cache is used.

Fixes phpGH-21052
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Feb 23, 2026
Since phpGH-15021 preloaded constants are propagated to compiled scripts. This is
problematic for file cache, which assumes all referenced zvals are either
persistently allocated or local to the current script. However, preloaded
constants live in shm as immutable, but not persistent.

To solve this, we'd need to duplicate propagated constants in the optimizer when
file cache is used. This is error prone given it needs to happen in many places.
It's debatable whether constant propagation is even correct in this case, as
running the preloaded script on a restart isn't guaranteed to produce the same
result.

Hence, avoid the issue for now by just not relying on preloaded symbols when
file cache is used.

Fixes phpGH-21052
iluuu1994 added a commit that referenced this pull request Feb 24, 2026
Since GH-15021 preloaded constants are propagated to compiled scripts. This is
problematic for file cache, which assumes all referenced zvals are either
persistently allocated or local to the current script. However, preloaded
constants live in shm as immutable, but not persistent.

To solve this, we'd need to duplicate propagated constants in the optimizer when
file cache is used. This is error prone given it needs to happen in many places.
It's debatable whether constant propagation is even correct in this case, as
running the preloaded script on a restart isn't guaranteed to produce the same
result.

Hence, avoid the issue for now by just not relying on preloaded symbols when
file cache is used.

Fixes GH-21052
Closes GH-21281
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.

4 participants