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

core: Implement FastAbs to avoid repeated os.Getwd calls #6687

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

francislavoie
Copy link
Member

Needs benchmarking

@francislavoie francislavoie added the under review 🧐 Review is pending before merging label Nov 12, 2024
filepath_unix.go Outdated Show resolved Hide resolved
filepath_windows.go Outdated Show resolved Hide resolved
Copy link
Member

@mholt mholt left a 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. 👍

@dunglas
Copy link
Collaborator

dunglas commented Nov 12, 2024

@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).

@mholt
Copy link
Member

mholt commented Nov 12, 2024

Gotcha. Good idea @dunglas !

@francislavoie Is there anything left to settle about the build tag/filename?

@dunglas
Copy link
Collaborator

dunglas commented Nov 12, 2024

@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.

@francislavoie
Copy link
Member Author

Apparently _unix.go is not an actual build constraint. See golang/go#51572

This is ready to go now.

@francislavoie francislavoie added this to the v2.9.0-beta.4 milestone Nov 13, 2024
@francislavoie francislavoie enabled auto-merge (squash) November 13, 2024 08:54
@francislavoie francislavoie merged commit 315715e into master Nov 13, 2024
33 checks passed
@francislavoie francislavoie deleted the fast-abs branch November 13, 2024 08:55
@francislavoie francislavoie added optimization 📉 Performance or cost improvements and removed under review 🧐 Review is pending before merging labels Nov 15, 2024
@francislavoie
Copy link
Member Author

francislavoie commented Nov 15, 2024

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 os.Getwd, calling it just once for the whole server.

This does mean plugins should never call os.Chdir (could cause bad things to happen with caddy.FastAbs) but I think it's pretty unlikely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization 📉 Performance or cost improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants