Skip to content
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

mount.RecursiveUnmount(): minor improvements #46

Merged
merged 2 commits into from
Oct 2, 2020

Conversation

thaJeztah
Copy link
Member

  • calculate "last mount" outside of the loop
  • omit empty "sub-errors" in error message

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

@kolyshkin

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

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
Collaborator

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

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
Member Author

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!

Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin kolyshkin merged commit 13322f1 into moby:master Oct 2, 2020
@thaJeztah thaJeztah deleted the last_mount branch October 2, 2020 21:10
@thaJeztah thaJeztah mentioned this pull request Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants