Skip to content

[RFC] Flip ns call scope lookup order #14529

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

Closed

Conversation

iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Jun 9, 2024

Inspired by #13632, the aim of this RFC is to improve the performance of global, internal function calls inside namespaced code without the need of prefixing all calls with \, or explicitly use-ing them. If this is proposed, it will likely be aimed at 9.0, which some migration path in 8.4, if it makes it in time.

Currently, a non-fq function call name is looked up in local scope, and then in global scope. For example, a call to var_dump() inside the App namespace will first look for \App\var_dump(), and only then for \var_dump(). It is a well-known optimization to prefix the call with \ to avoid the local lookup.

However, for internal calls this goes a few steps further. Particularly:

  • Some internal functions can be evaluated at compile-time if their arguments are known at compile-time.
  • Some internal functions have specialized opcodes that execute faster than function calls (e.g. strlen()).
  • Some internal functions have "frameless handlers" that don't require pushing a stack frame.
  • All internal function calls can avoid an initial function lookup by name and replace it with a cheaper offset into the function table.
  • Known arguments can be sent more efficiently because of known by-value/by-reference passing.

All of these optimizations are limited to functions known at compile-time. Because we don't know if var_dump() refers to \App\var_dump() or \var_dump(), we cannot apply any of these optimizations.

However, if we were to tweak the lookup to first check in global scope, and only then in local scope, we can assume that any function found in global scope is the function that will be executed at run-time. There are a couple of downsides:

  • Unqualified calls in namespaces will be slightly slower because they now involve checking global scope first. I believe that unqualified, global function calls are much more common than relative calls to functions in the same namespace, so this should still result in a net positive. You can still avoid this cost by including the local function with use. Functions that happen to share the name of a global function MUST be adjusted, or the wrong function will be called.
  • Introducing new functions in the global namespace now is a BC break for unqualified function calls to namespaced functions of the same name. This is unfortunate, but hopefully rare. Since new functions are only introduced in minor/major versions, this should be manageable.
  • I will consult internals next to see if this would break any use-cases (e.g. poor mans mocking, by declaring functions in the same namespace as some file).

Valgrind shows a 0.91% reduction in instructions for Symfony Demo. I'm not sure yet how this translates to real world performance.

Here's a small impact analysis: https://gist.github.com/iluuu1994/4b83481baac563f8f0d3204c697c5551 There are 484 namespaced functions shadowing global, internal functions in the top 1000 composer packages. However, most of these come from thecodingmachine/safe, whose entire purpose is offering safer wrappers around internal functions. Excluding these, there are only 20 such functions, which is surprisingly little.

@iluuu1994
Copy link
Member Author

Apparently, pretty much nobody was in favor of this approach, so I'm not going to be pursing it further.

@iluuu1994 iluuu1994 closed this Jun 13, 2024
@iluuu1994
Copy link
Member Author

I decided to raise this on the list anyway. This way, at least the analysis isn't lost.

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.

1 participant