Skip to content

Shrink some commonly used structs by reordering members #10880

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 3 commits into from
Mar 22, 2023

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Mar 19, 2023

Struct members require some alignment based on their type. This means that if a struct member is not aligned, there will be a hole created by the compiler in the struct, which is wasted space. This patch reorders some of the most commonly used structs, but in such a way that the fields which were in the same cache line still belong together. The only exception to this is exception_ignore_args, which was temporally not close to nearby members, and as such I placed it further up to close a hole.

On 64-bit Linux this gives us the following shrinks:

  • zend_op_array: 248 -> 240
  • zend_ssa_var: 56 -> 48
  • zend_ssa_var_info: 48 -> 40
  • php_core_globals: 672 -> 608
  • zend_executor_globals: 1824 -> 1792

On 32-bit, the sizes will either remain the same or will result in smaller shrinks.

There should be almost no BC impact. There is one BC caveat which can also be seen with the change to zend_executor_globals: if code initializes the struct in a way that's position dependent, then the order of the initialization needs to be changed too. This happened in Zend/zend_execute.c. If these kind of things are unacceptable, I can drop the ones which are externally most likely to be visible. (or we can drop the PR if this idea is considered unacceptable as a whole)
Perhaps this also needs an UPGRADING.INTERNALS note if this gets merged.

I don't know whether we want this kind of change, I'm not strongly married to the idea. This PR is only a first step.

Struct members require some alignment based on their type. This means
that if a struct member is not aligned, there will be a hole created by
the compiler in the struct, which is wasted space. This patch reorders
some of the most commonly used structs, but in such a way that the
fields which were in the same cache line still belong together.
The only exception to this is exception_ignore_args, which was
temporally not close to nearby members, and as such I placed
it further up to close a hole.

On 64-bit Linux this gives us the following shrinks:
* zend_op_array: 248 -> 240
* zend_ssa_var: 56 -> 48
* zend_ssa_var_info: 48 -> 40
* php_core_globals: 672 -> 608
* zend_executor_globals: 1824 -> 1792

On 32-bit, the sizes will either remain the same or will result in
smaller shrinks.
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

I personally don't think optimizing space for globals is super important, but I feel indifferent on changing it. The change for zend_op_array seems to save 4 bytes which seems useful. Same for the SSA structures.

@nielsdos
Copy link
Member Author

8 bytes ;)
And yeah, globals aren't very important. From a brief look there are quite a few globals of which we can shave off some bytes, but that's a much smaller save than these globals and therefore those are probably not worth the code churn.

@iluuu1994
Copy link
Member

8 bytes ;)

Ah, right 🙂

@nielsdos
Copy link
Member Author

Oh, one thing I forgot to mention regarding globals is that although the space save won't be impressive, it's good for reducing data cache pressure.

@iluuu1994
Copy link
Member

Also true. Grouping data that is frequently accessed together definitely makes sense.

@nielsdos
Copy link
Member Author

Changed to bool and added entry to UPGRADING.INTERNALS

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @nielsdos 🙂

@devnexen
Copy link
Member

would it be fair to ask another opinion (e.g. Dimtry) ?

@iluuu1994
Copy link
Member

@devnexen Sure 🙂

@devnexen devnexen requested a review from dstogov March 22, 2023 10:37
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.

This looks fine.

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.

5 participants