-
Notifications
You must be signed in to change notification settings - Fork 43
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
mountinfo: implement unescape() using strings.Replacer() #143
Conversation
Just cleaning up some old branches; not 100% sure about this one; wasn't sure how strict we wanted to be, and if simplifying the code is worth the slightly reduced performance |
go test -v -test.benchmem -count=10 -run ^$ -bench BenchmarkUnescape . go: downloading golang.org/x/sys v0.1.0 goos: linux goarch: arm64 pkg: github.com/moby/sys/mountinfo BenchmarkUnescape BenchmarkUnescape-10 1000000 1068 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 992292 1082 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1050 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1038 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1034 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1045 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1071 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1033 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1032 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1070 ns/op 640 B/op 31 allocs/op PASS ok github.com/moby/sys/mountinfo 10.653s Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
So, this mostly was because I saw this approach in another projefct, and liked how simple/clean it was. There are some downsides though; - performance looks to be _slightly_ less - we're no longer bothering with invalid escape sequences For the latter, I'm actually wondering how much validation we should do; should we take invalid escape sequences as literal strings? What does Linux itself do with these? I know in the past we added too much complication at times, and sometimes were validating things we should not care about (or even validating inconsistent with the host itself). Comparison between old `unescape()` and the new, using fstabUnescape.Replace(): Before: go test -v -test.benchmem -count=10 -run ^$ -bench BenchmarkUnescape . go: downloading golang.org/x/sys v0.1.0 goos: linux goarch: arm64 pkg: github.com/moby/sys/mountinfo BenchmarkUnescape BenchmarkUnescape-10 1000000 1068 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 992292 1082 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1050 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1038 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1034 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1045 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1071 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1033 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1032 ns/op 640 B/op 31 allocs/op BenchmarkUnescape-10 1000000 1070 ns/op 640 B/op 31 allocs/op PASS ok github.com/moby/sys/mountinfo 10.653s After go test -v -test.benchmem -count=10 -run ^$ -bench BenchmarkUnescape . goos: linux goarch: arm64 pkg: github.com/moby/sys/mountinfo BenchmarkUnescape BenchmarkUnescape-10 1000000 1141 ns/op 520 B/op 32 allocs/op BenchmarkUnescape-10 977841 1132 ns/op 520 B/op 32 allocs/op BenchmarkUnescape-10 1000000 1160 ns/op 520 B/op 32 allocs/op BenchmarkUnescape-10 901806 1131 ns/op 520 B/op 32 allocs/op BenchmarkUnescape-10 980247 1137 ns/op 520 B/op 32 allocs/op BenchmarkUnescape-10 988596 1135 ns/op 520 B/op 32 allocs/op BenchmarkUnescape-10 975658 1139 ns/op 520 B/op 32 allocs/op BenchmarkUnescape-10 934603 1161 ns/op 520 B/op 32 allocs/op BenchmarkUnescape-10 997353 1123 ns/op 520 B/op 32 allocs/op BenchmarkUnescape-10 986551 1131 ns/op 520 B/op 32 allocs/op PASS ok github.com/moby/sys/mountinfo 11.245s From the above: - new version is ~100 ns/op slower - new version has one more allocation (32 vs 31) - new version uses 120B less memory (520B vs 640B) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
b2d94c1
to
a437d73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting. I like the way it simplifies the code, and it seems the additional overhead is insignificant.
One small concern is current function interprets all escape sequences, while the new one is more limited. Need to take a look at the modern kernel, hope they haven't added more codes.
OK, we
In general, I would not rely on man pages in such a delicate subject. We have to drink raw and unfiltered C code from the kernel source tree. It looks like mountinfo is shown by show_mountinfo. Let's only look at the fields we're interested in:
Looking into fs-specific
Now, these analysis might be wrong and they are definitely incomplete as there are filesystems which source code is not in the vanilla kernel source tree (e.g. aufs), and they may have their own escaping rules. At the very least, this patch should add |
It looks like kernel uses Note though that the kernel always outputs 3 octal digits. I will see if I can simplify my code. |
I ended up with #144, PTAL |
Yes! I honestly don't recall where I found this approach, but I saw it somewhere and thought "if it doesn't add a lot of overhead, then it might be an option". FWIW: I don't really mind the existing code (it's still fairly readable, and if it's for performance, sometimes it's ok to have more verbose code for that), but thought I'd give it a try.
Thx! Will give it a look 👍 |
Closing in favour of #144 |
mountinfo: add benchmark for unescape
mountinfo: implement unescape() using strings.Replacer()
So, this mostly was because I saw this approach in another projefct, and liked
how simple/clean it was.
There are some downsides though;
For the latter, I'm actually wondering how much validation we should do; should
we take invalid escape sequences as literal strings? What does Linux itself do
with these?
I know in the past we added too much complication at times, and sometimes were
validating things we should not care about (or even validating inconsistent with
the host itself).
Comparison between old
unescape()
and the new, using fstabUnescape.Replace():Before:
After
From the above: