[go] runtime: add byte count parser for GOMEMLIMIT
Michael Knyszek (Gerrit)
Attention is currently required from: Austin Clements, Michael Pratt.
Patch set 13:Run-TryBot +1Trust +1
1 comment:
File src/runtime/string_test.go:
Patch Set #13, Line 458:
func TestParseByteCount(t *testing.T) {needs more tests.
To view, visit change 393400. To unsubscribe, or for help writing mail filters, visit settings.
Michael Pratt (Gerrit)
Attention is currently required from: Michael Knyszek, Austin Clements.
Patch set 18:Code-Review -1
4 comments:
File src/runtime/string.go:
// - KiB, MiB, GiB, TiB which represent binary IEC/ISO 80000 units, or
// - KB, MB, GB, TB which represent decimal SI units.FWIW, I remain unconvinced that we really need to support distinguishing between KiB and KB, or that users will actually select the one that they want.
I foresee lots of user CLs like
```
- GOMEMLIMIT=8GB
- + GOMEMLIMIT=8GiB
- ```
When folks realize they picked the wrong one.
Perhaps just support the GiB form for simplicity, but don't support GB at all to avoid ambiguity (hard error ideally).
Patch Set #18, Line 435:
for i := 0; i < len(s); i++ {Perhaps this is too complex to be worth it, but could we make atoi return int64 and the remainder of the string (or remainder start index), to share a bit of this?
File src/runtime/string_test.go:
How long until someone wants octal input? :P
Patch Set #18, Line 516:
{"12345x", 0, false},0x12345
To view, visit change 393400. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Austin Clements, Michael Pratt.
4 comments:
File src/runtime/string.go:
// - KiB, MiB, GiB, TiB which represent binary IEC/ISO 80000 units, or
// - KB, MB, GB, TB which represent decimal SI units.
FWIW, I remain unconvinced that we really need to support distinguishing between KiB and KB, or that […]
Done
Patch Set #18, Line 435:
for i := 0; i < len(s); i++ {
Perhaps this is too complex to be worth it, but could we make atoi return int64 and the remainder of […]
done
File src/runtime/string_test.go:
Patch Set #13, Line 458:
func TestParseByteCount(t *testing.T) {needs more tests.
Done
File src/runtime/string_test.go:
Patch Set #18, Line 516:
{"12345x", 0, false},0x12345
Done
To view, visit change 393400. To unsubscribe, or for help writing mail filters, visit settings.
Michael Pratt (Gerrit)
Attention is currently required from: Michael Knyszek, Austin Clements.
Patch set 19:Code-Review +2
Ingo Oeser (Gerrit)
Attention is currently required from: Michael Knyszek, Austin Clements.
2 comments:
Patchset:
Added a design question.
Optional units don't seem like a good option to me.
File src/runtime/string.go:
Patch Set #19, Line 429:
// In other words, an integer byte count with an optional unitWhy is the unit optional?
such a design usually lead to the error that users need to assume an unit (bytes) and might get it wrong (OS pages) or assume it is some factor (e.g. percentage like in GOGC).
Another problem is that one can accidentally assume this will always be numeric only and then suddenly someone is using GiB on it.
So can that part be re-considered?
To view, visit change 393400. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ingo Oeser, Austin Clements.
1 comment:
File src/runtime/string.go:
Patch Set #19, Line 429:
// In other words, an integer byte count with an optional unit
if you have concerns about the design, please comment on the GitHub issue (https://github.com/golang/go/issues/48409) and not in code review. that being said, I'll provide a response here, but I'm marking this as resolved. if you have further questions or comments please raise them on the issue, it's far more visible there. (in hindsight I should've asked Michael to post there too. I'll add an update later.)
such a design usually lead to the error that users need to assume an unit (bytes) and might get it wrong (OS pages) or assume it is some factor (e.g. percentage like in GOGC).
without a unit, I'm not sure why anything other than bytes would be assumed. there isn't a single standard library Go API that accepts an amount of memory as anything other than bytes.
Another problem is that one can accidentally assume this will always be numeric only and then suddenly someone is using GiB on it.
I don't understand this concern. if someone is used to seeing it without a unit, and then they see it with a unit, the unit coupled with the environment variable (GOMEMLIMIT) provide enough context to identify what's going on.
To view, visit change 393400. To unsubscribe, or for help writing mail filters, visit settings.
Michael Pratt (Gerrit)
Attention is currently required from: Ingo Oeser, Michael Knyszek, Austin Clements.
Patch set 25:Code-Review +2
Michael Knyszek (Gerrit)
Attention is currently required from: Ingo Oeser, Austin Clements.
Patch set 28:Run-TryBot +1
Michael Knyszek (Gerrit)
Attention is currently required from: Ingo Oeser, Austin Clements.
Patch set 30:Run-TryBot +1
Michael Knyszek (Gerrit)
Michael Knyszek submitted this change.
25 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
runtime: add byte count parser for GOMEMLIMIT
This change adds a parser for the GOMEMLIMIT environment variable's
input. This environment variable accepts a number followed by an
optional prefix expressing the unit. Acceptable units include
B, KiB, MiB, GiB, TiB, where *iB is a power-of-two byte unit.
For #48409.
Change-Id: I6a3b4c02b175bfcf9c4debee6118cf5dda93bb6f
Reviewed-on: https://go-review.googlesource.com/c/go/+/393400
TryBot-Result: Gopher Robot <go...@golang.org>
Reviewed-by: Michael Pratt <mpr...@google.com>
Run-TryBot: Michael Knyszek <mkny...@google.com>
---
M src/runtime/export_test.go
M src/runtime/string.go
M src/runtime/string_test.go
3 files changed, 249 insertions(+), 12 deletions(-)
diff --git a/src/runtime/export_test.go b/src/runtime/export_test.go
index 708da26..c364e5b 100644
--- a/src/runtime/export_test.go
+++ b/src/runtime/export_test.go
@@ -33,6 +33,7 @@
var Atoi = atoi
var Atoi32 = atoi32
+var ParseByteCount = parseByteCount
var Nanotime = nanotime
var NetpollBreak = netpollBreak
diff --git a/src/runtime/string.go b/src/runtime/string.go
index 8b20c93..845dcb5 100644
--- a/src/runtime/string.go
+++ b/src/runtime/string.go
@@ -351,14 +351,14 @@
}
const (
- maxUint = ^uint(0)
- maxInt = int(maxUint >> 1)
+ maxUint64 = ^uint64(0)
+ maxInt64 = int64(maxUint64 >> 1)
)
-// atoi parses an int from a string s.
+// atoi64 parses an int64 from a string s.
// The bool result reports whether s is a number
-// representable by a value of type int.
-func atoi(s string) (int, bool) {
+// representable by a value of type int64.
+func atoi64(s string) (int64, bool) {
if s == "" {
return 0, false
}
@@ -369,18 +369,18 @@
s = s[1:]
}
- un := uint(0)
+ un := uint64(0)
for i := 0; i < len(s); i++ {
c := s[i]
if c < '0' || c > '9' {
return 0, false
}
- if un > maxUint/10 {
+ if un > maxUint64/10 {
// overflow
return 0, false
}
un *= 10
- un1 := un + uint(c) - '0'
+ un1 := un + uint64(c) - '0'
if un1 < un {
// overflow
return 0, false
@@ -388,14 +388,14 @@
un = un1
}
- if !neg && un > uint(maxInt) {
+ if !neg && un > uint64(maxInt64) {
return 0, false
}
- if neg && un > uint(maxInt)+1 {
+ if neg && un > uint64(maxInt64)+1 {
return 0, false
}
- n := int(un)
+ n := int64(un)
if neg {
n = -n
}
@@ -403,15 +403,108 @@
return n, true
}
+// atoi is like atoi64 but for integers
+// that fit into an int.
+func atoi(s string) (int, bool) {
+ if n, ok := atoi64(s); n == int64(int(n)) {
+ return int(n), ok
+ }
+ return 0, false
+}
+
// atoi32 is like atoi but for integers
// that fit into an int32.
func atoi32(s string) (int32, bool) {
- if n, ok := atoi(s); n == int(int32(n)) {
+ if n, ok := atoi64(s); n == int64(int32(n)) {
return int32(n), ok
}
return 0, false
}
+// parseByteCount parses a string that represents a count of bytes.
+//
+// s must match the following regular expression:
+//
+// ^[0-9]+(([KMGT]i)?B)?$
+//
+// In other words, an integer byte count with an optional unit
+// suffix. Acceptable suffixes include one of
+// - KiB, MiB, GiB, TiB which represent binary IEC/ISO 80000 units, or
+// - B, which just represents bytes.
+//
+// Returns an int64 because that's what its callers want and recieve,
+// but the result is always non-negative.
+func parseByteCount(s string) (int64, bool) {
+ // The empty string is not valid.
+ if s == "" {
+ return 0, false
+ }
+ // Handle the easy non-suffix case.
+ last := s[len(s)-1]
+ if last >= '0' && last <= '9' {
+ n, ok := atoi64(s)
+ if !ok || n < 0 {
+ return 0, false
+ }
+ return n, ok
+ }
+ // Failing a trailing digit, this must always end in 'B'.
+ // Also at this point there must be at least one digit before
+ // that B.
+ if last != 'B' || len(s) < 2 {
+ return 0, false
+ }
+ // The one before that must always be a digit or 'i'.
+ if c := s[len(s)-2]; c >= '0' && c <= '9' {
+ // Trivial 'B' suffix.
+ n, ok := atoi64(s[:len(s)-1])
+ if !ok || n < 0 {
+ return 0, false
+ }
+ return n, ok
+ } else if c != 'i' {
+ return 0, false
+ }
+ // Finally, we need at least 4 characters now, for the unit
+ // prefix and at least one digit.
+ if len(s) < 4 {
+ return 0, false
+ }
+ power := 0
+ switch s[len(s)-3] {
+ case 'K':
+ power = 1
+ case 'M':
+ power = 2
+ case 'G':
+ power = 3
+ case 'T':
+ power = 4
+ default:
+ // Invalid suffix.
+ return 0, false
+ }
+ m := uint64(1)
+ for i := 0; i < power; i++ {
+ m *= 1024
+ }
+ n, ok := atoi64(s[:len(s)-3])
+ if !ok || n < 0 {
+ return 0, false
+ }
+ un := uint64(n)
+ if un > maxUint64/m {
+ // Overflow.
+ return 0, false
+ }
+ un *= m
+ if un > uint64(maxInt64) {
+ // Overflow.
+ return 0, false
+ }
+ return int64(un), true
+}
+
//go:nosplit
func findnull(s *byte) int {
if s == nil {
diff --git a/src/runtime/string_test.go b/src/runtime/string_test.go
index 4eda12c..1ea7f5e 100644
--- a/src/runtime/string_test.go
+++ b/src/runtime/string_test.go
@@ -454,3 +454,126 @@
}
}
}
+
+func TestParseByteCount(t *testing.T) {
+ for _, test := range []struct {
+ in string
+ out int64
+ ok bool
+ }{
+ // Good numeric inputs.
+ {"1", 1, true},
+ {"12345", 12345, true},
+ {"012345", 12345, true},
+ {"98765432100", 98765432100, true},
+ {"9223372036854775807", 1<<63 - 1, true},
+
+ // Good trivial suffix inputs.
+ {"1B", 1, true},
+ {"12345B", 12345, true},
+ {"012345B", 12345, true},
+ {"98765432100B", 98765432100, true},
+ {"9223372036854775807B", 1<<63 - 1, true},
+
+ // Good binary suffix inputs.
+ {"1KiB", 1 << 10, true},
+ {"05KiB", 5 << 10, true},
+ {"1MiB", 1 << 20, true},
+ {"10MiB", 10 << 20, true},
+ {"1GiB", 1 << 30, true},
+ {"100GiB", 100 << 30, true},
+ {"1TiB", 1 << 40, true},
+ {"99TiB", 99 << 40, true},
+
+ // Good zero inputs.
+ //
+ // -0 is an edge case, but no harm in supporting it.
+ {"-0", 0, true},
+ {"0", 0, true},
+ {"0B", 0, true},
+ {"0KiB", 0, true},
+ {"0MiB", 0, true},
+ {"0GiB", 0, true},
+ {"0TiB", 0, true},
+
+ // Bad inputs.
+ {"", 0, false},
+ {"-1", 0, false},
+ {"a12345", 0, false},
+ {"a12345B", 0, false},
+ {"12345x", 0, false},
+ {"0x12345", 0, false},
+
+ // Bad numeric inputs.
+ {"9223372036854775808", 0, false},
+ {"9223372036854775809", 0, false},
+ {"18446744073709551615", 0, false},
+ {"20496382327982653440", 0, false},
+ {"18446744073709551616", 0, false},
+ {"18446744073709551617", 0, false},
+ {"9999999999999999999999", 0, false},
+
+ // Bad trivial suffix inputs.
+ {"9223372036854775808B", 0, false},
+ {"9223372036854775809B", 0, false},
+ {"18446744073709551615B", 0, false},
+ {"20496382327982653440B", 0, false},
+ {"18446744073709551616B", 0, false},
+ {"18446744073709551617B", 0, false},
+ {"9999999999999999999999B", 0, false},
+
+ // Bad binary suffix inputs.
+ {"1Ki", 0, false},
+ {"05Ki", 0, false},
+ {"10Mi", 0, false},
+ {"100Gi", 0, false},
+ {"99Ti", 0, false},
+ {"22iB", 0, false},
+ {"B", 0, false},
+ {"iB", 0, false},
+ {"KiB", 0, false},
+ {"MiB", 0, false},
+ {"GiB", 0, false},
+ {"TiB", 0, false},
+ {"-120KiB", 0, false},
+ {"-891MiB", 0, false},
+ {"-704GiB", 0, false},
+ {"-42TiB", 0, false},
+ {"99999999999999999999KiB", 0, false},
+ {"99999999999999999MiB", 0, false},
+ {"99999999999999GiB", 0, false},
+ {"99999999999TiB", 0, false},
+ {"555EiB", 0, false},
+
+ // Mistaken SI suffix inputs.
+ {"0KB", 0, false},
+ {"0MB", 0, false},
+ {"0GB", 0, false},
+ {"0TB", 0, false},
+ {"1KB", 0, false},
+ {"05KB", 0, false},
+ {"1MB", 0, false},
+ {"10MB", 0, false},
+ {"1GB", 0, false},
+ {"100GB", 0, false},
+ {"1TB", 0, false},
+ {"99TB", 0, false},
+ {"1K", 0, false},
+ {"05K", 0, false},
+ {"10M", 0, false},
+ {"100G", 0, false},
+ {"99T", 0, false},
+ {"99999999999999999999KB", 0, false},
+ {"99999999999999999MB", 0, false},
+ {"99999999999999GB", 0, false},
+ {"99999999999TB", 0, false},
+ {"99999999999TiB", 0, false},
+ {"555EB", 0, false},
+ } {
+ out, ok := runtime.ParseByteCount(test.in)
+ if test.out != out || test.ok != ok {
+ t.Errorf("parseByteCount(%q) = (%v, %v) want (%v, %v)",
+ test.in, out, ok, test.out, test.ok)
+ }
+ }
+}
To view, visit change 393400. To unsubscribe, or for help writing mail filters, visit settings.