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
[24.0 backport] pkg/fileutils: GetTotalUsedFds: reduce allocations #45856
Conversation
Commit 8d56108 moved this function from the generic (no build-tags) fileutils.go to a unix file, adding "freebsd" to the build-tags. This likely was a wrong assumption (as other files had freebsd build-tags). FreeBSD's procfs does not mention `/proc/<pid>/fd` in the manpage, and we don't test FreeBSD in CI, so let's drop it, and make this a Linux-only file. While updating also dropping the import-tag, as we're planning to move this file internal to the daemon. Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 252e94f) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
go test -bench ^BenchmarkGetTotalUsedFds$ -run ^$ ./pkg/fileutils/ goos: linux goarch: arm64 pkg: github.com/docker/docker/pkg/fileutils BenchmarkGetTotalUsedFds-5 149272 7896 ns/op 945 B/op 20 allocs/op Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 03390be) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Use File.Readdirnames instead of os.ReadDir, as we're only interested in the number of files, and results don't have to be sorted. Before: BenchmarkGetTotalUsedFds-5 149272 7896 ns/op 945 B/op 20 allocs/op After: BenchmarkGetTotalUsedFds-5 153517 7644 ns/op 408 B/op 10 allocs/op Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit eaa9494) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Linux 6.2 and up (commit [f1f1f2569901ec5b9d425f2e91c09a0e320768f3][1]) provides a fast path for the number of open files for the process. From the [Linux docs][2]: > The number of open files for the process is stored in 'size' member of > `stat()` output for /proc/<pid>/fd for fast access. [1]: torvalds/linux@f1f1f25 [2]: https://docs.kernel.org/filesystems/proc.html#proc-pid-fd-list-of-symlinks-to-open-files This patch adds a fast-path for Kernels that support this, and falls back to the slow path if the Size fields is zero. Comparing on a Fedora 38 (kernel 6.2.9-300.fc38.x86_64): Before/After: go test -bench ^BenchmarkGetTotalUsedFds$ -run ^$ ./pkg/fileutils/ BenchmarkGetTotalUsedFds 57264 18595 ns/op 408 B/op 10 allocs/op BenchmarkGetTotalUsedFds 370392 3271 ns/op 40 B/op 3 allocs/op Note that the slow path has 1 more file-descriptor, due to the open file-handle for /proc/<pid>/fd during the calculation. Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit ec79d0f) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
I just ran into the containerd log changes myself in #45857; personally I think it's more idiomatic to fix the conflict in the cherry-picked commit itself, rather than synthesize a single-file cherry-pick to make a downstream cherry-pick 'clean.' |
Yeah, always a bit on the fence; in this case; all the cherry-picks where squeaky clean after the first commit, and it may be more clear in history, but yeah YMMV I guess. |
// GetTotalUsedFds Returns the number of used File Descriptors by | ||
// reading it via /proc filesystem. | ||
func GetTotalUsedFds() int { | ||
name := fmt.Sprintf("/proc/%d/fd", os.Getpid()) |
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.
Surprised you used Sprintf here :)
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.
Haha, yeah I was doubting, but in the total scheme of things, it was slightly more readable than strconv and path.Join combined
pkg/fileutils: GetTotalUsedFds(): don't pretend to support FreeBSD
Commit 8d56108 moved this function from
the generic (no build-tags) fileutils.go to a unix file, adding "freebsd"
to the build-tags.
This likely was a wrong assumption (as other files had freebsd build-tags).
FreeBSD's procfs does not mention
/proc/<pid>/fd
in the manpage, andwe don't test FreeBSD in CI, so let's drop it, and make this a Linux-only
file.
While updating also dropping the import-tag, as we're planning to move
this file internal to the daemon.
pkg/fileutils: add BenchmarkGetTotalUsedFds
pkg/fileutils: GetTotalUsedFds: reduce allocations
Use File.Readdirnames instead of os.ReadDir, as we're only interested in
the number of files, and results don't have to be sorted.
Before:
After:
pkg/fileutils: GetTotalUsedFds(): use fast-path for Kernel 6.2 and up
Linux 6.2 and up (commit f1f1f2569901ec5b9d425f2e91c09a0e320768f3)
provides a fast path for the number of open files for the process.
From the Linux docs:
This patch adds a fast-path for Kernels that support this, and falls back
to the slow path if the Size fields is zero.
Comparing on a Fedora 38 (kernel 6.2.9-300.fc38.x86_64):
Before/After:
Note that the slow path has 1 more file-descriptor, due to the open
file-handle for /proc//fd during the calculation.
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)