Skip to content
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

Chore: pointer refactor #449

Closed
wants to merge 6 commits into from
Closed

Chore: pointer refactor #449

wants to merge 6 commits into from

Conversation

withinboredom
Copy link
Collaborator

This takes advantage of the smart-pointer stuff from #442 and a small optimization on go_register_variables. Through some benchmarks, this is just a tiny bit faster (but within a margin of error, so, at the very least, just as fast).

It introduces a (possibly contentious) idea that any C memory created in Go, must be released in Go -- which is new.

I'm also opening a PR with just the optimization in case we don't like this idea.

Beyond this, FrankenPHP cannot be optimized further without improvements in stack switching by Go itself.

I did some benchmarks; there is a slight (but barely detectable) improvement over the other PR:

branch time per request (1.5 million requests - 95% quantile) error
main 60.39ms 1.2ms
chore/pointer-refactor 59.20ms 1.3ms
chore/optimize 60.05ms 1.2ms

As you can see, for performance, this is a micro-optimization; however, I believe it can improve code maintenance since it delineates which language owns which bits of memory.

@dunglas
Copy link
Owner

dunglas commented Dec 29, 2023

I like the idea, IIUC but I suspect that this will increase the memory usage if the PHP script is a long-running one.

For instance, we currently free the memory allocated for $_SERVER entries before starting the execution of the PHP script itself.

With this patch, we will do it after. If the PHP script is long and slow, we keep the memory for some time for nothing. WDYT?

@withinboredom
Copy link
Collaborator Author

we currently free the memory allocated for $_SERVER entries before starting the execution of the PHP script itself.

With this patch, we will do it after. If the PHP script is long and slow, we keep the memory for some time for nothing.

Oof, that is a good point that I didn't consider. I was thinking of how to start and finish the response faster, which would indeed use more memory, especially on longer-running scripts.

Anyway, I will close this as I was hoping for more performance gains than what was realized and I doubt the memory usage is worth the small gains.

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