Skip to content

Fix GH-11715: opcache.interned_strings_buffer either has no effect or… #11717

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 3 commits into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Jul 17, 2023

… opcache_get_status() / phpinfo() is wrong

There are a couple of oddities.

  1. The interned strings buffer comprises the whole hashtable
    datastructure.
    Therefore, it seems that the interned strings buffer size is the size of
    only said table. However, in the current code it also includes the size
    of the zend_accel_shared_globals.

  2. ZCSG(interned_strings).end is computed starting from the accelerator
    globals struct itself. I would expect it to start from the part where
    the interned strings table starts.

  3. When computing the size, it is done using
    ZCSG(interned_strings).end - ZCSG(interned_strings).start. However,
    this does not include the uint32_t slots array because
    ZCSG(interned_strings).start points after that array.

This patch corrrects these 3 points.

… or opcache_get_status() / phpinfo() is wrong

There are a couple of oddities.

1) The interned strings buffer comprises the whole hashtable
   datastructure.
   Therefore, it seems that the interned strings buffer size is the size of
   only said table. However, in the current code it also includes the size
   of the zend_accel_shared_globals.

2) ZCSG(interned_strings).end is computed starting from the accelerator
   globals struct itself. I would expect it to start from the part where
   the interned strings table starts.

3) When computing the used size, it is done using
   ZCSG(interned_strings).end - ZCSG(interned_strings).start. However,
   this does not include the uin32_t slots array because
   ZCSG(interned_strings).start pointers after that array.

This patch corrrects these 3 points.
@Girgias Girgias requested a review from arnaud-lb July 17, 2023 04:09
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. Thank you!

When merging to 8.2, could you update the MAX_INTERNED_STRINGS_BUFFER_SIZE macro to ((zend_long)((UINT32_MAX-PLATFORM_ALIGNMENT-sizeof(zend_accel_shared_globals))/(1024*1024)))?

@nielsdos nielsdos closed this in ee3f932 Jul 21, 2023
@nielsdos
Copy link
Member Author

Thanks, I've done the additional change you noted for 8.2+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opcache.interned_strings_buffer either has no effect or opcache_get_status() / phpinfo() is wrong
2 participants