rangefeed: improve assertions #118265
rangefeed: improve assertions #118265
Conversation
|
This change is |
This will include the log tags from the context, aiding with debugging. Involves a bunch of context plumbing, and removing some assertion tests. A couple of panics were left in, where it didn't appear natural to plumb through a context. Epic: none Release note: None
`String()` is only implemented for a pointer receiver, so the formatting breaks otherwise. Epic: none Release note: None
erikgrinaker
force-pushed
the
rangefeed-panics
branch
from
b2fae3d to
401d70b
Compare
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)
pkg/kv/kvserver/rangefeed/resolved_timestamp.go line 300 at r2 (raw file):
op := op err := errors.AssertionFailedf( "resolved timestamp %s equal to or above timestamp of operation %v", rts.resolvedTS, &op)
Was this actually causing the op param to move to the heap when not on the assertion path? I'd think it would only do so if we were passing &op like we were doing in #118413.
Here's a test showing that on master:
➜ git rev-parse HEAD
f71b21dbd904e1a6a7ccd1f767db769c48476e2a
➜ pwd
/Users/nathan/Go/src/github.com/cockroachdb/cockroach/pkg/kv/kvserver/rangefeed
➜ fgrep 'equal to or above timestamp of operation' resolved_timestamp.go
"resolved timestamp %s equal to or above timestamp of operation %v", rts.resolvedTS, op)
➜ go build -gcflags='-m' 2>&1 | grep 'heap' | grep 'resolved_timestamp.go:29'
./resolved_timestamp.go:296:76: rts.resolvedTS escapes to heap
./resolved_timestamp.go:296:89: op escapes to heap
➜ fgrep 'equal to or above timestamp of operation' resolved_timestamp.go
"resolved timestamp %s equal to or above timestamp of operation %v", rts.resolvedTS, &op)
➜ go build -gcflags='-m' 2>&1 | grep 'heap' | grep 'resolved_timestamp.go:29'
./resolved_timestamp.go:292:23: moved to heap: op
./resolved_timestamp.go:296:76: rts.resolvedTS escapes to heap
pkg/kv/kvserver/rangefeed/registry_test.go line 512 at r1 (raw file):
} func TestRegistryPublishAssertsPopulatedInformation(t *testing.T) {
Would it be worth testing for log.Fatal instead of losing this test coverage?
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/rangefeed/resolved_timestamp.go line 300 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Was this actually causing the
opparam to move to the heap when not on the assertion path? I'd think it would only do so if we were passing&oplike we were doing in #118413.Here's a test showing that on master:
➜ git rev-parse HEAD f71b21dbd904e1a6a7ccd1f767db769c48476e2a ➜ pwd /Users/nathan/Go/src/github.com/cockroachdb/cockroach/pkg/kv/kvserver/rangefeed ➜ fgrep 'equal to or above timestamp of operation' resolved_timestamp.go "resolved timestamp %s equal to or above timestamp of operation %v", rts.resolvedTS, op) ➜ go build -gcflags='-m' 2>&1 | grep 'heap' | grep 'resolved_timestamp.go:29' ./resolved_timestamp.go:296:76: rts.resolvedTS escapes to heap ./resolved_timestamp.go:296:89: op escapes to heap ➜ fgrep 'equal to or above timestamp of operation' resolved_timestamp.go "resolved timestamp %s equal to or above timestamp of operation %v", rts.resolvedTS, &op) ➜ go build -gcflags='-m' 2>&1 | grep 'heap' | grep 'resolved_timestamp.go:29' ./resolved_timestamp.go:292:23: moved to heap: op ./resolved_timestamp.go:296:76: rts.resolvedTS escapes to heap
Yeah, looks like op would always escape to the heap here. I think printf arguments generally always do, there's some analysis of why here: golang/go#8618 (comment)
pkg/kv/kvserver/rangefeed/registry_test.go line 512 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Would it be worth testing for log.Fatal instead of losing this test coverage?
I'm kind of ambivalent towards testing assertions. It's sort of like adding tests for tests -- occasionally useful, but generally not worth the cost.
I see that we have log.SetExitFunc() to override the fatal, so I can port these over if you feel like it's worthwhile.
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/rangefeed/resolved_timestamp.go line 300 at r2 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
Yeah, looks like
opwould always escape to the heap here. I thinkprintfarguments generally always do, there's some analysis of why here: golang/go#8618 (comment)
Discussed on Slack. For now, we'll just go with the following until we can bottom out on understanding this better:
err := errors.AssertionFailedf("resolved timestamp %s equal to or above timestamp of operation %s", rts.resolvedTS, op.String())
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/rangefeed/resolved_timestamp.go line 300 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Discussed on Slack. For now, we'll just go with the following until we can bottom out on understanding this better:
err := errors.AssertionFailedf("resolved timestamp %s equal to or above timestamp of operation %s", rts.resolvedTS, op.String())
Ran a benchmark for the happy path:
op: does not allocate.&op: allocates.op := op; &op: does not allocate.
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/rangefeed/resolved_timestamp.go line 300 at r2 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
Ran a benchmark for the happy path:
op: does not allocate.&op: allocates.op := op; &op: does not allocate.
Of course, op.String() does allocate, lol.
op := op; &op it is then.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
|
TFTR! bors r+ |
|
Build succeeded: |
rangefeed: use
log.Fatalinstead of panicThis will include the log tags from the context, aiding with debugging. Involves a bunch of context plumbing, and removing some assertion tests.
A couple of panics were left in, where it didn't appear natural to plumb through a context.
rangefeed: use pointer receiver in MVCCLogicalOp assertion
String()is only implemented for a pointer receiver, so the formatting breaks otherwise.Epic: none
Release note: None