Skip to content

Conversation

tback
Copy link

@tback tback commented Mar 17, 2018

The scoreboard structs are locked so concise readings can be acquired.

@bukka: I decided against implementing fpm_scoreboard_copy for now. The way I did it avoids explicit memory management because the scoreboard copies are put on the stack.

I don't know exactly what php version introduced the fpm status page, but I know the code was present in 5.5 already.

Fixes https://bugs.php.net/bug.php?id=76109

The scoreboard structs are locked so concise readings can be acquired.
Fixes https://bugs.php.net/bug.php?id=76109
@@ -92,6 +97,9 @@ int fpm_status_handle_request(void) /* {{{ */
return 1;
}

struct fpm_scoreboard_proc_s procs[scoreboard_p->nprocs];
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is gonna work on all supported compilers. We also target C89 so definitions has to be at the beginning of block.

Copy link
Member

Choose a reason for hiding this comment

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

Considering that VLA is C99, you can't really use it though.

@bukka
Copy link
Member

bukka commented Mar 18, 2018

@tback I think that memory allocation is a small overhead and this (status) is definitely not a perf critical path so I wouldn't worry about that. As I mentioned, you can't use VLA so there is not much options anyway... :) The fpm_scoreboard_copy is really preferred in here though.

It will go to 7.1 so if you could target it, that would be great! Thanks for your work!

@tback
Copy link
Author

tback commented Mar 19, 2018

Thanks @bukka. I will close this PR and open another one when I have something.

@tback tback closed this Mar 19, 2018
@tback tback deleted the fix-scoreboard-access branch March 20, 2018 09:53
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.

2 participants