pkg/fileutils: GetTotalUsedFds: reduce allocations#45848
Merged
thaJeztah merged 4 commits intomoby:masterfrom Jun 30, 2023
Merged
pkg/fileutils: GetTotalUsedFds: reduce allocations#45848thaJeztah merged 4 commits intomoby:masterfrom
thaJeztah merged 4 commits intomoby:masterfrom
Conversation
thaJeztah
commented
Jun 29, 2023
akerouanton
reviewed
Jun 29, 2023
pkg/fileutils/fileutils_unix.go
Outdated
| log.G(context.TODO()).Errorf("Error opening /proc/%d/fd: %s", os.Getpid(), err) | ||
| } else { | ||
| return len(fds) | ||
| nameCount, err := readDirnamesCount(fmt.Sprintf("/proc/%d/fd", os.Getpid())) |
Member
There was a problem hiding this comment.
From https://docs.kernel.org/filesystems/proc.html#proc-pid-fd-list-of-symlinks-to-open-files:
The number of open files for the process is stored in 'size' member of stat() output for /proc//fd for fast access.
Member
There was a problem hiding this comment.
Per @corhere: only on kernel 6.4+. We should use this as a fast path, and fall back to readdir(2)
Contributor
There was a problem hiding this comment.
For future reference: the fast path was added to the kernel in torvalds/linux@f1f1f25
3569810 to
f05cc5d
Compare
corhere
reviewed
Jun 29, 2023
f05cc5d to
01cdff2
Compare
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>
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>
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>
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>
01cdff2 to
ec79d0f
Compare
thaJeztah
commented
Jun 29, 2023
Comment on lines
+49
to
+51
| // Note that the slow path has 1 more file-descriptor, due to the open | ||
| // file-handle for /proc/<pid>/fd during the calculation. | ||
| return fdCount |
Member
Author
There was a problem hiding this comment.
This was a fun one; "wait, why is there an off-by-one?" ahhhh... wait... gotcha.
vvoland
approved these changes
Jun 30, 2023
neersighted
approved these changes
Jun 30, 2023
This was referenced Jun 30, 2023
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
5 participants
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)