Skip to content

[24.0 backport] pkg/fileutils: GetTotalUsedFds: reduce allocations#45856

Merged
cpuguy83 merged 5 commits intomoby:24.0from
thaJeztah:24.0_backport_improve_fd_count
Jun 30, 2023
Merged

[24.0 backport] pkg/fileutils: GetTotalUsedFds: reduce allocations#45856
cpuguy83 merged 5 commits intomoby:24.0from
thaJeztah:24.0_backport_improve_fd_count

Conversation

@thaJeztah
Copy link
Copy Markdown
Member


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

pkg/fileutils: add BenchmarkGetTotalUsedFds

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

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:

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

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:

The number of open files for the process is stored in 'size' member of
stat() output for /proc//fd for fast access.

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//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)

(very) partial backport of 74da6a6
and ab35df4

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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>
@thaJeztah thaJeztah added status/2-code-review area/daemon Core Engine kind/refactor PR's that refactor, or clean-up code labels Jun 30, 2023
@thaJeztah thaJeztah added this to the 24.0.3 milestone Jun 30, 2023
@neersighted
Copy link
Copy Markdown
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.'

@thaJeztah
Copy link
Copy Markdown
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.

// 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())
Copy link
Copy Markdown
Member

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 :)

Copy link
Copy Markdown
Member Author

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

@cpuguy83 cpuguy83 merged commit be50480 into moby:24.0 Jun 30, 2023
@thaJeztah thaJeztah deleted the 24.0_backport_improve_fd_count branch June 30, 2023 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/daemon Core Engine kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants