-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
core: Implement FastAbs to avoid repeated os.Getwd calls #6687
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the build tag (would need more time to investigate myself, but it sounds right, the _unix implies not Windows or any other non-Unix OS) -- but overall this LGTM. I wonder if we should add to our docs that, for performance reasons, we don't support chdir
on the process while it's running.
I would definitely want to see some benchmark though to get a sense for the improvement here to justify this. 👍
@mholt For chdir, if there is a use case, we could even provide our own wrapper that will just refresh the wd variable after the change (it will not cover some use cases like an external library using directly os.Chdir, the C function or the syscall, but it should cover most use cases). |
Gotcha. Good idea @dunglas ! @francislavoie Is there anything left to settle about the build tag/filename? |
@mholt the similar change in FrankenPHP improves our benchmark performance by about 7%, which is quite significant. As it's not exactly the same case, it may not apply there, but it will likely improve performance for PHP FPM and static files users significantly. Creating a Go benchmark should be enough to measure that. |
5d556b7
to
78fa3aa
Compare
Apparently This is ready to go now. |
For ref, this was a collab with dunglas/frankenphp#1154, this change was mirrored in FrankenPHP. This was something like a 7% performance increase by avoiding the syscalls to This does mean plugins should never call |
Needs benchmarking