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.

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

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

In tests.

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

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):

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):

Done.

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)

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):

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):

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.

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

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

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):

I mean the check that the builder was not copied.

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

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

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):

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.

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

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

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):

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?

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):

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

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

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

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

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