Skip to content

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

Merged
francislavoie merged 3 commits intomasterfrom
fast-abs
Nov 13, 2024
Merged

core: Implement FastAbs to avoid repeated os.Getwd calls#6687
francislavoie merged 3 commits intomasterfrom
fast-abs

Conversation

@francislavoie
Copy link
Member

Needs benchmarking

@francislavoie francislavoie added the under review 🧐 Review is pending before merging label Nov 12, 2024
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
@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 php/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.

mohammed90 pushed a commit to cedricziel/caddy that referenced this pull request Aug 29, 2025
…#6687)

* core: Implement FastAbs to avoid repeated os.Getwd calls

* Lint

* Rename files
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