[24.0 backport] pkg/fileutils: GetTotalUsedFds: reduce allocations#45856
Merged
[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>
Member
|
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.' |
Member
Author
|
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. |
neersighted
approved these changes
Jun 30, 2023
cpuguy83
approved these changes
Jun 30, 2023
cpuguy83
reviewed
Jun 30, 2023
| // 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()) |
Member
There was a problem hiding this comment.
Surprised you used Sprintf here :)
Member
Author
There was a problem hiding this comment.
Haha, yeah I was doubting, but in the total scheme of things, it was slightly more readable than strconv and path.Join combined
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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>/fdin 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)