Remove regexp use #40

kolyshkin · 2022-04-28T03:40:01Z

Copy/paste from the most important commit:
size: stop using regexp

Using a regular expression brings in the whole regexp and regexp/syntax
packages, which increase the resulting binary size by about 130K
(Go 1.18.1, Linux/amd64).

Besides, regular expressions are generally slow and incur some
initialization overhead (to compile all global regexp.MustComplile
variables). This, unlike the size difference, is not the main motivation
for this commit, but it feels like it should have also been mentioned.

A quick benchmark comparison shows a huge improvement (again, this is
not why this is done, nevertheless it pleases the eye):

name         old time/op    new time/op    delta
ParseSize-4    10.6µs ± 3%     2.6µs ±29%  -75.10%  (p=0.002 n=6+6)

name         old alloc/op   new alloc/op   delta
ParseSize-4    3.26kB ± 0%    0.20kB ± 0%  -93.75%  (p=0.000 n=7+6)

name         old allocs/op  new allocs/op  delta
ParseSize-4      72.0 ± 0%      26.0 ± 0%  -63.89%  (p=0.000 n=7+6)

Compatibility note: I chose to accept (rather than error out) input
strings like ".4 Gb" as this is valid and makes sense, and the inability
to process such input by the old implementation is more about the
limitation of regex being used, rather than a decision to error out on
those.

For the rest, please see commit messages.

thaJeztah · 2022-04-28T11:38:14Z

size_test.go

@@ -128,6 +129,8 @@ func TestRAMInBytes(t *testing.T) {
 	assertSuccessEquals(t, 32, RAMInBytes, "32.3")
 	tmp := 32.3 * MiB
 	assertSuccessEquals(t, int64(tmp), RAMInBytes, "32.3 mb")
+	tmp = 0.3 * MiB
+	assertSuccessEquals(t, int64(tmp), RAMInBytes, "0.3MB")


Should this be 0.3 <space> MB ?

oh, never mind; expected is the first argument 😞 we should swap those; always confuses me if assertions use <expected> <actual>

thaJeztah · 2022-04-28T11:57:14Z

size_test.go

@@ -99,11 +99,11 @@ func TestFromHumanSize(t *testing.T) {
 	assertSuccessEquals(t, 32.5*KB, FromHumanSize, "32.5 kB")
 	assertSuccessEquals(t, 32, FromHumanSize, "32.5 B")
 	assertSuccessEquals(t, 300, FromHumanSize, "0.3 K")
+	assertSuccessEquals(t, 300, FromHumanSize, ".3kB")


Slightly on the fence if we want to accept .<number> (and <number>.) or not 🤔

Came up with some additional test-cases (TBD if the valid/invalids are in the right one);

tests with leading/trailing whitespace (I think generally we should be "ok" with those, and trim before use)

tests with trailing . (should be an error)

tests with multiple (but valid) units, e.g. bb (looks like those aren't detected)

TBD: multiple spaces between number and unit (probably should be an error)

TBD: number with ., but no number following (should we accept those?)

func TestFromHumanSize(t *testing.T) { assertSuccessEquals(t, 0, FromHumanSize, " 0") assertSuccessEquals(t, 0, FromHumanSize, " 0b") assertSuccessEquals(t, 0, FromHumanSize, " 0B") assertSuccessEquals(t, 0, FromHumanSize, " 0 B") assertSuccessEquals(t, 0, FromHumanSize, "0 ") assertSuccessEquals(t, 0, FromHumanSize, "0b ") assertSuccessEquals(t, 0, FromHumanSize, "0B ") assertSuccessEquals(t, 0, FromHumanSize, "0 B ") assertSuccessEquals(t, 0, FromHumanSize, "0") assertSuccessEquals(t, 0, FromHumanSize, "0b") assertSuccessEquals(t, 0, FromHumanSize, "0B") assertSuccessEquals(t, 0, FromHumanSize, "0 B") assertSuccessEquals(t, 32, FromHumanSize, "32") assertSuccessEquals(t, 32, FromHumanSize, "32b") assertSuccessEquals(t, 32, FromHumanSize, "32B") assertSuccessEquals(t, 32, FromHumanSize, "32") assertSuccessEquals(t, 32*KB, FromHumanSize, "32k") assertSuccessEquals(t, 32*KB, FromHumanSize, "32K") assertSuccessEquals(t, 32*KB, FromHumanSize, "32kb") assertSuccessEquals(t, 32*KB, FromHumanSize, "32Kb") assertSuccessEquals(t, 32*MB, FromHumanSize, "32Mb") assertSuccessEquals(t, 32*GB, FromHumanSize, "32Gb") assertSuccessEquals(t, 32*TB, FromHumanSize, "32Tb") assertSuccessEquals(t, 32*PB, FromHumanSize, "32Pb") assertSuccessEquals(t, 32.5*KB, FromHumanSize, "32.5kB") assertSuccessEquals(t, 32.5*KB, FromHumanSize, "32.5 kB") assertSuccessEquals(t, 32, FromHumanSize, "32.5 B") assertSuccessEquals(t, 300, FromHumanSize, "0.3 K") assertSuccessEquals(t, 300, FromHumanSize, ".3kB") assertError(t, FromHumanSize, "") assertError(t, FromHumanSize, ".") assertError(t, FromHumanSize, ". ") assertError(t, FromHumanSize, " ") assertError(t, FromHumanSize, " ") assertError(t, FromHumanSize, " .") assertError(t, FromHumanSize, " . ") assertError(t, FromHumanSize, "0.") assertError(t, FromHumanSize, "0. ") assertError(t, FromHumanSize, "0.b") assertError(t, FromHumanSize, "0.B") assertError(t, FromHumanSize, "-0") assertError(t, FromHumanSize, "-0b") assertError(t, FromHumanSize, "-0B") assertError(t, FromHumanSize, "-0 b") assertError(t, FromHumanSize, "-0 B") assertError(t, FromHumanSize, "-32") assertError(t, FromHumanSize, "-32b") assertError(t, FromHumanSize, "-32B") assertError(t, FromHumanSize, "-32 b") assertError(t, FromHumanSize, "-32 B") assertError(t, FromHumanSize, "32.") assertError(t, FromHumanSize, "32.b") assertError(t, FromHumanSize, "32.B") assertError(t, FromHumanSize, "32. b") assertError(t, FromHumanSize, "32. B") assertError(t, FromHumanSize, "32b.") assertError(t, FromHumanSize, "32B.") assertError(t, FromHumanSize, "32 b.") assertError(t, FromHumanSize, "32 B.") assertError(t, FromHumanSize, "32 bb") assertError(t, FromHumanSize, "32 BB") assertError(t, FromHumanSize, "32 b b") assertError(t, FromHumanSize, "32 B B") assertError(t, FromHumanSize, "32 b") assertError(t, FromHumanSize, "32 B") assertError(t, FromHumanSize, "hello") assertError(t, FromHumanSize, "-32") assertError(t, FromHumanSize, " 32 ") assertError(t, FromHumanSize, "32m b") assertError(t, FromHumanSize, "32bm") }

So, as to what should pass and what should fail, I think we should follow the rules of strconv.ParseFloat (plus our own way of handling suffixes). This is why I decided we should pass .3 as it's a valid floating point number in those programming languages/environments I am familiar with.

Adding your tests now.

This is why I decided we should pass .3 as it's a valid floating point number in those programming languages/environments I am familiar wit

Ah, gotcha, yes, that makes sense.

With the leading/trailing . I was a bit concerned about those coming from "idk" (accidental things), so was veering on the "safe side".

assertSuccessEquals(t, 0, FromHumanSize, " 0") assertSuccessEquals(t, 0, FromHumanSize, " 0b") assertSuccessEquals(t, 0, FromHumanSize, " 0B") assertSuccessEquals(t, 0, FromHumanSize, " 0 B")

Neither the old nor the new code tolerates extra leading or trailing space. We can certainly relax that, although my original intent was to replicate the functionality as-is (with the exception of accepting strings like ".33 GB" or "345. B", as described in the last commit).

thaJeztah · 2022-04-28T12:06:18Z

go.mod

@@ -0,0 +1,3 @@
+module github.com/docker/go-units


I'm thinking if this repository should also live in the moby org; wondering what's best; add a go.mod already, or wait with adding until after we moved; wdyt?

In either case, may be better to do it separate

Right, let's move this to moby first.

The go.mod is added for selfish reasons only -- so I can actually run go test.

👍

reminds me, I also want to look at https://github.com/tonistiigi/units, and see where the gaps (and overlaps) are between the two, as both end up in our dependency tree; perhaps we can unify them

(I know there's some possibly "bad" choices in docker/go-units, so maybe there's reasons for one over the other, but 🤷‍♂️)

Filed #41 about move to under moby org. Once done we can move from there.

Looking into https://github.com/tonistiigi/units

OK, Tonis' repo is about printing/output, and does not cover conversion/input.

Yup; it's "the other side of go-units", so mostly complementary; but in the same area, so it could make sense to have these together (not urgent, but an option).

I checked with Tonis, and he wasn't against, so it's an option we can consider (not "MUST", but can)

it could make sense to have these together

Perhaps it's better to keep it separate, as they solve complementary, but different tasks.

thaJeztah

Thanks! overall looks good; left some comments;

also, can you drop the first commit for now (adding the go.mod)?

size_test.go

Such sizes are quite valid but were never tested. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Shows on my laptop (6 iterations processed by benchstat): name time/op ParseSize-4 10.6µs ± 3% name alloc/op ParseSize-4 3.26kB ± 0% name allocs/op ParseSize-4 72.0 ± 0% Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This way, error messages will show correct line information. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>

Using a regular expression brings in the whole regexp and regexp/syntax packages, which increase the resulting binary size by about 130K (Go 1.18.1, Linux/amd64). Besides, regular expressions are generally slow and incur some initialization overhead (to compile all global regexp.MustComplile variables). This, unlike the size difference, is not the main motivation for this commit, but it feels like it should have also been mentioned. A quick benchmark comparison shows a huge improvement (again, this is not why this is done, nevertheless it pleases the eye): name old time/op new time/op delta ParseSize-4 10.6µs ± 3% 2.6µs ±29% -75.10% (p=0.002 n=6+6) name old alloc/op new alloc/op delta ParseSize-4 3.26kB ± 0% 0.20kB ± 0% -93.75% (p=0.000 n=7+6) name old allocs/op new allocs/op delta ParseSize-4 72.0 ± 0% 26.0 ± 0% -63.89% (p=0.000 n=7+6) Compatibility note: As a result, we are now a but more relaxed to the input, allowing e.g. ".4 Gb", or "-0", or "234. B", following the rules of strconv.ParseFloat. It seems that those were previously rejected as a result of a regex being used, not deliberately. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2022-05-16T16:43:31Z

also, can you drop the first commit for now (adding the go.mod)?

Done

thaJeztah

LGTM, thanks!

We only use a single method, RAMInBytes, which can be easily implemented locally. Do that (based on docker/go-units#40, so the implementation is fully backward-compatible, except for the addition of treating -1), and remove docker/go-units dependency. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

We only use a single method, RAMInBytes, which can be easily implemented locally. Do that (based on docker/go-units#40, so the implementation is fully backward-compatible, except for the addition of treating -1), and remove docker/go-units dependency. This implementation relies on strings.Cut() which is only available since Go 1.18. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Mostly to include docker/go-units#40 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

... to improve performance by not using regular expressions [1]. However, the potential reduction in binary size is lost because Toolbx already uses the 'regexp' package to check if a string might be the ID of an image or a valid container name. [1] go-units commit 737572633c434ce2 docker/go-units@737572633c434ce2 docker/go-units#40

... to improve performance by not using regular expressions [1]. However, the potential reduction in binary size is lost because Toolbx already uses the 'regexp' package to check if a string might be the ID of an image or a valid container name. [1] go-units commit 737572633c434ce2 docker/go-units@737572633c434ce2 docker/go-units#40 containers#1420

thaJeztah reviewed Apr 28, 2022

View reviewed changes

kolyshkin force-pushed the no-regex branch from 53b72ac to 189caea Compare April 28, 2022 19:40

This was referenced Apr 28, 2022

RegExp: ignore case flag #39

Closed

Do not use regexp opencontainers/runc#3460

Merged

thaJeztah requested changes May 12, 2022

View reviewed changes

size_test.go Show resolved Hide resolved

size_test.go Outdated Show resolved Hide resolved

size_test.go Show resolved Hide resolved

kolyshkin and others added 5 commits May 16, 2022 09:42

size_test: add tests for 0.3 + suffix

af07f36

Such sizes are quite valid but were never tested. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

size_test: add parseSize benchmark

1d23ffa

Shows on my laptop (6 iterations processed by benchstat): name time/op ParseSize-4 10.6µs ± 3% name alloc/op ParseSize-4 3.26kB ± 0% name allocs/op ParseSize-4 72.0 ± 0% Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

size_test: add t.Helper annotations

54c3e55

This way, error messages will show correct line information. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

size_test: add more tests

cec4960

Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the no-regex branch from 189caea to 7375726 Compare May 16, 2022 16:42

kolyshkin requested a review from thaJeztah May 16, 2022 16:43

thaJeztah approved these changes May 17, 2022

View reviewed changes

thaJeztah merged commit e682442 into docker:master May 17, 2022

kolyshkin mentioned this pull request Jun 6, 2022

Release 0.5.0 (or 0.4.1) #42

Closed

kolyshkin added a commit to kolyshkin/runc that referenced this pull request Sep 1, 2022

vendor: bump docker/units to v0.5.0

1ddc10b

Mostly to include docker/go-units#40 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

debarshiray mentioned this pull request Dec 14, 2023

build: Bump github.com/docker/go-units to 0.5.0 containers/toolbox#1420

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove regexp use #40

Remove regexp use #40

kolyshkin commented Apr 28, 2022

thaJeztah Apr 28, 2022

thaJeztah Apr 28, 2022

thaJeztah Apr 28, 2022

thaJeztah Apr 28, 2022

kolyshkin Apr 28, 2022

thaJeztah Apr 28, 2022

kolyshkin Apr 28, 2022

thaJeztah Apr 28, 2022

kolyshkin Apr 28, 2022

thaJeztah Apr 28, 2022

kolyshkin Apr 28, 2022

kolyshkin Apr 28, 2022

thaJeztah Apr 28, 2022 •

edited

kolyshkin Apr 29, 2022

thaJeztah left a comment

kolyshkin commented May 16, 2022

thaJeztah left a comment

Remove regexp use #40

Remove regexp use #40

Conversation

kolyshkin commented Apr 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah Apr 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

kolyshkin commented May 16, 2022

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Apr 28, 2022 •

edited