Skip to content

Add PHP-FPM memory peak to scoreboard #14153

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

Conversation

flavioheleno
Copy link
Contributor

This PR adds a new entry to PHP-FPM's scoreboard to track peak memory usage.

This is useful to track memory usage and help better tune PHP/PHP-FPM configuration.

Performance impact should be minimal as it is only updated in request end (fpm_request_end).

@flavioheleno flavioheleno requested a review from bukka as a code owner May 6, 2024 14:59
@bukka
Copy link
Member

bukka commented May 12, 2024

You might want to read: #10600 (comment) . TLDR not sure if this is really useful as it just reports peak of the last request...

@flavioheleno
Copy link
Contributor Author

The idea behind this change is to record the largest peak in memory usage between requests.

From what I can understand from the code, the fpm_request_end function is executed for all requests and the treatment done by fpm_scoreboard_update_commit is what guarantees the recording of the largest peak (similar to the treatment done for active max), or am I wrong?

In general, we now have a better view of memory consumption, regardless of how often the extractor consumes data (as is the case when we use the data provided by full, which only provides data for the last request).

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

I had a better look now and after thinking about it I think it actually makes sense to add it. We have got already some similar fields and I see the use case here. I will try to do a bit more testing but it should make it to 8.4

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

I have done some testing and it all works fine.

@SakiTakamachi @NattyNarwhal Can one of you please give me a 👍 so I can merge to 8.4.

@NattyNarwhal
Copy link
Member

Not familiar with FPM, but tests seem OK for me on macOS/arm64. gentoo/ppc64le I get a pass on an XFAIL test, but it's unrelated (FPM: Buffered worker output decorated log with multiple continuous messages (stdout/stderr mixed) [sapi/fpm/tests/log-bwd-multiple-msgs-stdout-stderr.phpt] (warn: XFAIL section but test passes)).

The change itself looks pretty minor to me in terms of impact yet pretty useful. I trust your judgement here.

@bukka bukka closed this in 67aac59 Aug 24, 2024
@flavioheleno flavioheleno deleted the feat/fpm-memory-peak branch August 26, 2024 12:06
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.

3 participants