Skip to content

Adjust static inline functions which don't inline #18454

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
wants to merge 1 commit into from

Conversation

morrisonlevi
Copy link
Contributor

@morrisonlevi morrisonlevi commented Apr 29, 2025

These are found by adding -Winline on gcc. The flag is accepted on clang for compatibility with gcc, but it doesn't do anything.

Mostly, this removes inline from the declarations. For local static functions, there is no point in adding inline. This stems from either a misunderstanding of what it means, or alternatively is a holdover from ancient times where it may have meant something (note it still means something in a header file, but not a source file). A compiler is free to inline a static function, and specifying inline doesn't make the compiler inline the function. These pointless inlines make it harder to analyze failed inlining, so I remove them.

However, for select functions, this changes them to zend_always_inline instead. This is because these functions didn't inline, but probably should for performance reasons. For instance, _class_exists_impl is used in multiple frameless function handlers. I think in these cases, the authors were hoping they'd be inlined due to fact that are important for performance. There's also _php_search_array, which is used with constant args on a variety of functions. It fails to inline due to its size, but if it did inline, it would cut large amounts of code out, so forcing the inlining seemed worth it.

There's also i_get_exception_base which was removed. Again, the compiler is free to inline zend_get_exception_base if it wants to, and static inline doesn't make the compiler inline it. This function often failed to inline in my -Winline report. Alternatively this could be made zend_always_inline, but since this is related to exceptions--which are supposed to be rare to begin with--it seemed more prudent to not force the compiler to inline it.


This work is split out from #15075, so that's why it might seem like déjà vu.

There are still some -Winline failures, but they are in things I'd prefer not to change such as pcre or xxhash.

These are found by adding `-Winline` on gcc. The flag is accepted on
clang for compatibility with gcc, but it doesn't do anything.

Mostly, this removes `inline` from the declarations. For local static
functions, there is no point in adding `inline`. This stems from
either a misunderstanding of what it means, or alternatively is a
holdover from ancient times where it may have meant something. A
compiler is free to inline a static function, and specifying inline
doesn't make the compiler inline the function. It does however make
it harder to due analysis of failed inlining, so I remove them.

However, for select functions, this changes them `zend_always_inline`
instead. This is because these functions didn't inline, but probably
should for performance reasons. For instance, `_class_exists_impl` is
used in multiple frameless function handlers. I think in these cases,
the authors were _hoping_ they'd be inlined due to fact that are
important for performance. There's also `_php_search_array`, which is
used with constant args on a variety of functions. It fails to inline
due its size, but if it _did_ inline, it would cut large amounts of
code out, so forcing the inlining seemed worth it.
@dstogov
Copy link
Member

dstogov commented Apr 29, 2025

There's also i_get_exception_base which was removed. Again, the compiler is free to inline zend_get_exception_base if it wants to

C compiler won't inline zend_get_exception_base, because it's not static.

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

inline is a hint to C compiler. Every implementation may treat it differently and it doesn't have to perform inlining.

Removing common hints just because GCC decided not to inline (in some build) is wrong. It may decide not-to inline in DEBUG build and inline in RELEASE.

@morrisonlevi
Copy link
Contributor Author

I don't care to argue, even though I think you are wrong on at least some points.

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.

2 participants