Skip to content

Fix GH-8646: Memory leak PHP FPM 8.1 #10783

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
Closed

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Mar 4, 2023

Fixes GH-8646
See #8646 for thorough discussion.

/cc @iluuu1994

Interned strings that hold class entries can get a corresponding slot in map_ptr for the CE cache. map_ptr works like a bump allocator: there is a counter which increases to allocate the next slot in the map.

For class name strings in non-opcache we have:

  • on startup: permanent + interned
  • on request: interned

For class name strings in opcache we have:

  • on startup: permanent + interned
  • on request: either not interned at all, which we can ignore because they won't get a CE cache entry. Or they were already permanent + interned. Or we get a new permanent + interned string in the opcache persistence code

Notice that the map_ptr layout always has the permanent strings first, and the request strings after. In non-opcache, a request string may get a slot in map_ptr, and that interned request string gets destroyed at the end of the request. The corresponding map_ptr slot can thereafter never be used again. This causes map_ptr to keep reallocating to larger and larger sizes.

We solve it as follows:
We can check whether we had any interned request strings, which only happens in non-opcache. If we have any, we reset map_ptr to the last permanent string. We can't lose any permanent strings because of map_ptr's layout.

@nielsdos nielsdos requested a review from iluuu1994 March 4, 2023 23:09
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.

The test looks very nice and reliable 🙂 LGTM. Let's verify this by Dmitry, hopefully our analysis makes sense.

@dstogov
Copy link
Member

dstogov commented Mar 6, 2023

By design map_ptr slots might be created only during PHP startup or during storing something in opcache.
Somehow this was broken by 315f409, and this was the original problem.

It seems, that you've found a simple way to fix this by calling zend_map_ptr_reset() at the end of request (when opcache is not installed).
I tried to find some edge cases, but it seems the fix is fine.

I would just propose to move the fixing code into zend_deactivate().

@nielsdos
Copy link
Member Author

nielsdos commented Mar 6, 2023

Thank you very much for the review!
Yes, that commit wanted to get rid of some special cases but accidentally introduced this issue.

I would just propose to move the fixing code into zend_deactivate().

I will do that after work tonight. Thanks!

Fixes phpGH-8646
See php#8646 for thorough discussion.

Interned strings that hold class entries can get a corresponding slot in map_ptr for the CE cache.
map_ptr works like a bump allocator: there is a counter which increases to allocate the next slot in the map.

For class name strings in non-opcache we have:
  - on startup: permanent + interned
  - on request: interned
For class name strings in opcache we have:
  - on startup: permanent + interned
  - on request: either not interned at all, which we can ignore because they won't get a CE cache entry
                or they were already permanent + interned
                or we get a new permanent + interned string in the opcache persistence code

Notice that the map_ptr layout always has the permanent strings first, and the request strings after.
In non-opcache, a request string may get a slot in map_ptr, and that interned request string
gets destroyed at the end of the request. The corresponding map_ptr slot can thereafter never be used again.
This causes map_ptr to keep reallocating to larger and larger sizes.

We solve it as follows:
We can check whether we had any interned request strings, which only happens in non-opcache.
If we have any, we reset map_ptr to the last permanent string.
We can't lose any permanent strings because of map_ptr's layout.
@nielsdos
Copy link
Member Author

nielsdos commented Mar 6, 2023

Moved the code as requested :)

@nielsdos nielsdos self-assigned this Mar 6, 2023
@nielsdos nielsdos closed this in ff62d11 Mar 7, 2023
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.

3 participants