hlc: improve/fix the representation of timestamps #43185

knz · 2019-12-16T15:29:19Z

Requested by @petermattis in #42250 (review)
Informs #43183.

Previously, a negative timestamp would be printed with a negative sign
in the middle, for example 0.-000000123 or -123.-00000456.
This patch fixes this by only emitting the sign once.

Additionally, this patch simplifies zero timestamps to just "0".

Release note: None

cockroach-teamcity · 2019-12-16T15:29:28Z

This change is

nvanbenschoten

Reviewed 30 of 30 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @petermattis)

pkg/util/hlc/timestamp.go, line 42 at r1 (raw file):

	// The main problem with the original code was that it would put
	// a negative sign in the middle (after the decimal point) if
	// the value happened to be negative.

When do we see negative timestamps in practice?

pkg/util/hlc/timestamp.go, line 57 at r1 (raw file):

		// expressed in nanoseconds
		timeS := strconv.FormatUint(u, 10)
		if len(timeS) < 10 {

Can we detect this condition ahead of time so that we can use strconv.AppendUint and avoid the intermediate buffer?

pkg/util/hlc/timestamp.go, line 88 at r1 (raw file):

	buf = strconv.AppendInt(buf, int64(t.Logical), 10)
	// Convert the byte array to a string without a copy. We follow the
	// examples of strings.Builder here.

Could we use strings.Builder with an initial .Grow(12) to avoid needing to have any unsafe code here?

pkg/util/hlc/timestamp_test.go, line 111 at r1 (raw file):

		exp string
	}{
		{makeTS(0, 0), "0,0"},

Some of these cases should probably have non-zero logical components.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @petermattis)

pkg/util/hlc/timestamp.go, line 42 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

When do we see negative timestamps in practice?

In tests.

pkg/util/hlc/timestamp.go, line 57 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Can we detect this condition ahead of time so that we can use strconv.AppendUint and avoid the intermediate buffer?

Not so clear, no. We still need to pad the value on the left. AppendUint does not allow it.
The only fast path I can see is when the number of digits is exactly 9, but that seems extremely unlikely in practice.

pkg/util/hlc/timestamp.go, line 88 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Could we use strings.Builder with an initial .Grow(12) to avoid needing to have any unsafe code here?

Using a strings.Builder forces an extra check in every of its .Write methods. What do we gain from avoiding the unsafe cast?

pkg/util/hlc/timestamp_test.go, line 111 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Some of these cases should probably have non-zero logical components.

Done.

petermattis

The format change . I'll leave it to you and @nvanbenschoten to work out the implementation discussion.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @petermattis)

nvanbenschoten

Reviewed 1 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz)

pkg/util/hlc/timestamp.go, line 57 at r1 (raw file):

Previously, knz (kena) wrote…

Not so clear, no. We still need to pad the value on the left. AppendUint does not allow it.
The only fast path I can see is when the number of digits is exactly 9, but that seems extremely unlikely in practice.

We can't figure out these cases using modulus and integer division arithmetic and then turn the WallTime formatting into two strconv.AppendUint calls interleaved with a few conditions?

pkg/util/hlc/timestamp.go, line 88 at r1 (raw file):

Previously, knz (kena) wrote…

Using a strings.Builder forces an extra check in every of its .Write methods. What do we gain from avoiding the unsafe cast?

Because it would hide this complexity. I'm ok leaving this as is if you are, but I'm a little surprised that you'd opt for this.

Also, when you say "forces an extra check", you don't mean the bounds check, right? Because the appends here are still going to need to perform bounds checks.

knz

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

pkg/util/hlc/timestamp.go, line 57 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We can't figure out these cases using modulus and integer division arithmetic and then turn the WallTime formatting into two strconv.AppendUint calls interleaved with a few conditions?

I dont' see how that's either more readable or faster, let alone both.

pkg/util/hlc/timestamp.go, line 88 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Because it would hide this complexity. I'm ok leaving this as is if you are, but I'm a little surprised that you'd opt for this.

Also, when you say "forces an extra check", you don't mean the bounds check, right? Because the appends here are still going to need to perform bounds checks.

I mean the check that the builder was not copied.

nvanbenschoten

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz)

pkg/util/hlc/timestamp.go, line 57 at r1 (raw file):

Previously, knz (kena) wrote…

I dont' see how that's either more readable or faster, let alone both.

I'm thinking something like this: https://play.golang.org/p/YeGFi5naP3K

In my opinion, that's more readable (fewer conditions, appending in order, etc.) and is likely faster as well. I think it would be worthwhile to benchmark.

pkg/util/hlc/timestamp.go, line 88 at r1 (raw file):

Previously, knz (kena) wrote…

I mean the check that the builder was not copied.

Hm, I think the best argument for this is that it opens up the use of strconv.AppendInt and strconv.AppendUint.

pkg/util/hlc/timestamp.go, line 43 at r5 (raw file):

	// a negative sign in the middle (after the decimal point) if
	// the value happened to be negative.
	buf := make([]byte, 0, 12)

Where did you get 12 for this capacity? Just curious.

nvanbenschoten

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz)

pkg/util/hlc/timestamp.go, line 57 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'm thinking something like this: https://play.golang.org/p/YeGFi5naP3K

In my opinion, that's more readable (fewer conditions, appending in order, etc.) and is likely faster as well. I think it would be worthwhile to benchmark.

I went ahead and did so with the following script: https://play.golang.org/p/8_RDCznlkwC. Here are the results:

name         old time/op    new time/op    delta
ToString-16    68.4ns ± 2%    60.3ns ± 2%  -11.84%  (p=0.000 n=10+10)

name         old alloc/op   new alloc/op   delta
ToString-16     24.0B ± 0%     16.0B ± 0%  -33.33%  (p=0.000 n=10+10)

name         old allocs/op  new allocs/op  delta
ToString-16      2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.000 n=10+10)

pkg/util/hlc/timestamp.go, line 43 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Where did you get 12 for this capacity? Just curious.

FWIW I needed to bump this in my benchmark to avoid an extra allocation. This should be tuned to avoid a second alloc in most cases. Is that where 12 came from?

knz

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz and @nvanbenschoten)

pkg/util/hlc/timestamp.go, line 57 at r1 (raw file):

In my opinion, that's more readable (fewer conditions, appending in order, etc.) and is likely faster as well.

I assume you wanted me to ignore the ugly numDigits function 😆
But I'll take it. Everyone likes a good old fashioned fastLog10 function anyway.

pkg/util/hlc/timestamp.go, line 43 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

FWIW I needed to bump this in my benchmark to avoid an extra allocation. This should be tuned to avoid a second alloc in most cases. Is that where 12 came from?

It's the value sufficient to avoid an allocation in our tests, as they last less than 10 seconds in the common case.

knz

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz and @nvanbenschoten)

pkg/util/hlc/timestamp.go, line 57 at r1 (raw file):

Previously, knz (kena) wrote…

In my opinion, that's more readable (fewer conditions, appending in order, etc.) and is likely faster as well.

I assume you wanted me to ignore the ugly numDigits function 😆
But I'll take it. Everyone likes a good old fashioned fastLog10 function anyway.

(Also FYI regarding your gist, the go compiler did constant propagation so that skewed your benchmark positively. )

Previously, a negative timestamp would be printed with a negative sign in the middle, for example `0.-000000123` or `-123.-00000456`. This patch fixes this by only emitting the sign once. Additionally, this patch simplifies zero timestamps to just `"0"`. Release note: None

knz · 2019-12-19T19:10:06Z

RFAL

nvanbenschoten

Reviewed 3 of 3 files at r6.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale)

knz · 2019-12-20T14:41:23Z

TFYRs!

bors r+

@petermattis

43185: hlc: improve/fix the representation of timestamps r=knz a=knz Requested by @petermattis in #42250 (review) Informs #43183. Previously, a negative timestamp would be printed with a negative sign in the middle, for example `0.-000000123` or `-123.-00000456`. This patch fixes this by only emitting the sign once. Additionally, this patch simplifies zero timestamps to just `"0"`. Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]>

craig · 2019-12-20T15:12:08Z

Build succeeded

GitHub CI (Cockroach)

knz requested review from nvanbenschoten and petermattis December 16, 2019 15:29

knz force-pushed the 20191216-fix-hlc-string branch from 81f4fce to acec06a Compare December 16, 2019 16:11

knz mentioned this pull request Dec 16, 2019

storage: improve the txnmeta and ts pretty-printers #43183

Open

knz force-pushed the 20191216-fix-hlc-string branch from acec06a to 140cff3 Compare December 16, 2019 17:08

nvanbenschoten reviewed Dec 16, 2019

View reviewed changes

knz commented Dec 16, 2019

View reviewed changes

knz force-pushed the 20191216-fix-hlc-string branch from 140cff3 to 74c1d62 Compare December 16, 2019 17:23

petermattis approved these changes Dec 16, 2019

View reviewed changes

nvanbenschoten reviewed Dec 16, 2019

View reviewed changes

knz commented Dec 17, 2019

View reviewed changes

nvanbenschoten reviewed Dec 19, 2019

View reviewed changes

knz commented Dec 19, 2019

View reviewed changes

knz force-pushed the 20191216-fix-hlc-string branch 3 times, most recently from 5e6e438 to b7a8ff8 Compare December 19, 2019 19:04

knz force-pushed the 20191216-fix-hlc-string branch from b7a8ff8 to 0240455 Compare December 19, 2019 19:09

nvanbenschoten approved these changes Dec 20, 2019

View reviewed changes

craig bot merged commit 0240455 into cockroachdb:master Dec 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hlc: improve/fix the representation of timestamps #43185

hlc: improve/fix the representation of timestamps #43185

knz commented Dec 16, 2019 •

edited

Loading

cockroach-teamcity commented Dec 16, 2019

nvanbenschoten left a comment

knz left a comment

petermattis left a comment

nvanbenschoten left a comment

knz left a comment

nvanbenschoten left a comment

nvanbenschoten left a comment

knz left a comment

knz left a comment

knz commented Dec 19, 2019

nvanbenschoten left a comment

knz commented Dec 20, 2019

craig bot commented Dec 20, 2019

hlc: improve/fix the representation of timestamps #43185

hlc: improve/fix the representation of timestamps #43185

Conversation

knz commented Dec 16, 2019 • edited Loading

cockroach-teamcity commented Dec 16, 2019

nvanbenschoten left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

petermattis left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

knz commented Dec 19, 2019

nvanbenschoten left a comment

Choose a reason for hiding this comment

knz commented Dec 20, 2019

craig bot commented Dec 20, 2019

Build succeeded

knz commented Dec 16, 2019 •

edited

Loading