Skip to content

mount.RecursiveUnmount(): minor improvements #46

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.

Sign up for GitHub

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

Merged
kolyshkin merged 2 commits into from
Oct 2, 2020
Merged

mount.RecursiveUnmount(): minor improvements #46

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
53d4513
mount.RecursiveUnmount() calculate "last mount" outside loop
thaJeztah Oct 2, 2020
9ffc9f4
mount.RecursiveUnmount(): omit empty "sub-errors" in error message
thaJeztah Oct 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions mount/mount_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,18 @@ func RecursiveUnmount(target string) error {
return len(mounts[i].Mountpoint) > len(mounts[j].Mountpoint)
})

var suberr error
var (
suberr error
lastMount = len(mounts) - 1
Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Oct 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, not sure how smart the compiler would be, and if this change would be a no-op; if so, I can drop this commit.

Copy link
Copy Markdown
Collaborator

@kolyshkin kolyshkin Oct 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think len() is a very fast operation in Go, as it just returns the length from the internal structure that represents a slice.

A benchmark would be helpful to find out for sure

Copy link
Copy Markdown
Collaborator

@kolyshkin kolyshkin Oct 2, 2020 •
edited by thaJeztah
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is negligible (63 vs 64 ns per run), but it makes the code more readable so I think we can keep it.

Test case:

package mountinfo

import "testing"

func BenchmarkLenInLoop(b *testing.B) {
	mounts, err := GetMounts(nil)
	if err != nil {
		b.Fatal(err)
	}

	var last *Info
	b.ResetTimer()
	for n := 0; n < b.N; n++ {
		for i, m := range mounts {
			if i == len(mounts)-1 { // last mount
				last = m
			}
		}
	}
	b.Log("Last mount " + last.Mountpoint)
}

func BenchmarkLenOutOfLoop(b *testing.B) {
	mounts, err := GetMounts(nil)
	if err != nil {
		b.Fatal(err)
	}

	var (
		last         *Info
		lastMountIdx = len(mounts) - 1
	)
	b.ResetTimer()
	for n := 0; n < b.N; n++ {
		for i, m := range mounts {
			if i == lastMountIdx { // last mount
				last = m
			}
		}
	}
	b.Log("Last mount " + last.Mountpoint)
}

Results:

$ go test -bench Len -benchtime 10s .
goos: linux
goarch: amd64
pkg: github.com/moby/sys/mountinfo
BenchmarkLenInLoop-8      	186409401	        64.3 ns/op
--- BENCH: BenchmarkLenInLoop-8
    bench_test.go:20: Last mount /run/media/kir/8885b6f8-2cc0-4416-b5e9-dfecb3137d9a
    bench_test.go:20: Last mount /run/media/kir/8885b6f8-2cc0-4416-b5e9-dfecb3137d9a
    bench_test.go:20: Last mount /run/media/kir/8885b6f8-2cc0-4416-b5e9-dfecb3137d9a
    bench_test.go:20: Last mount /run/media/kir/8885b6f8-2cc0-4416-b5e9-dfecb3137d9a
    bench_test.go:20: Last mount /run/media/kir/8885b6f8-2cc0-4416-b5e9-dfecb3137d9a
    bench_test.go:20: Last mount /run/media/kir/8885b6f8-2cc0-4416-b5e9-dfecb3137d9a
BenchmarkLenOutOfLoop-8   	191245992	        63.2 ns/op
--- BENCH: BenchmarkLenOutOfLoop-8
    bench_test.go:41: Last mount /run/media/kir/8885b6f8-2cc0-4416-b5e9-dfecb3137d9a
    bench_test.go:41: Last mount /run/media/kir/8885b6f8-2cc0-4416-b5e9-dfecb3137d9a
    bench_test.go:41: Last mount /run/media/kir/8885b6f8-2cc0-4416-b5e9-dfecb3137d9a
    bench_test.go:41: Last mount /run/media/kir/8885b6f8-2cc0-4416-b5e9-dfecb3137d9a
    bench_test.go:41: Last mount /run/media/kir/8885b6f8-2cc0-4416-b5e9-dfecb3137d9a
    bench_test.go:41: Last mount /run/media/kir/8885b6f8-2cc0-4416-b5e9-dfecb3137d9a
PASS
ok  	github.com/moby/sys/mountinfo	36.921s

Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Oct 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, that's not much of a difference; thanks for testing!

)
for i, m := range mounts {
err = Unmount(m.Mountpoint)
if err != nil {
if i == len(mounts)-1 { // last mount
return fmt.Errorf("%w (possible cause: %s)", err, suberr)
if i == lastMount {
if suberr != nil {
return fmt.Errorf("%w (possible cause: %s)", err, suberr)
}
return err
}
// This is a submount, we can ignore the error for now,
// the final unmount will fail if this is a real problem.
Expand Down