Merge #88735 #88911

88735: roachtest: run clearrange test with range tombstones r=erikgrinaker a=aliher1911 Previously clear range tests were run with default server settings. Range tombstones are disabled by default so new functionality in schema change commands is not execised. This commit adds test variant that enables range tombstones. Release note: None Fixes #86752 88911: hlc: improve `Timestamp.IsEmpty()` performance r=sumeerbhola a=erikgrinaker `Timestamp.IsEmpty()` saw a significant performance regression with Go 1.19, in particular with empty timestamps, apparently due to the compiler's use of `memeqbody` for struct comparisons. On `EncodeMVCCKey` benchmarks, the empty timestamp case showed a ~50% regression. This patch changes `IsEmpty()` to avoid struct comparison, instead comparing the member fields directly. This yields a significant performance improvement (using Go 1.19): ``` name old time/op new time/op delta TimestampIsEmpty/empty-24 6.90ns ± 2% 0.79ns ± 0% -88.56% (p=0.000 n=10+9) TimestampIsEmpty/walltime-24 2.97ns ± 1% 0.75ns ± 1% -74.90% (p=0.000 n=10+10) TimestampIsEmpty/all-24 2.97ns ± 1% 0.75ns ± 1% -74.77% (p=0.000 n=10+10) ``` This in turn significantly improves `EncodeMVCCKey` performance: ``` name old time/op new time/op delta EncodeMVCCKey/key=empty/ts=empty-24 22.0ns ± 0% 15.7ns ± 5% -28.67% (p=0.000 n=9+10) EncodeMVCCKey/key=empty/ts=walltime-24 21.2ns ± 0% 17.8ns ± 0% -16.42% (p=0.000 n=10+10) EncodeMVCCKey/key=empty/ts=walltime+logical-24 21.8ns ± 0% 18.6ns ± 0% -15.02% (p=0.000 n=10+10) EncodeMVCCKey/key=empty/ts=all-24 22.2ns ± 0% 18.8ns ± 0% -15.07% (p=0.000 n=9+9) EncodeMVCCKey/key=short/ts=empty-24 22.3ns ± 0% 15.8ns ± 0% -29.06% (p=0.000 n=8+10) EncodeMVCCKey/key=short/ts=walltime-24 21.2ns ± 0% 18.0ns ± 0% -15.02% (p=0.000 n=9+8) EncodeMVCCKey/key=short/ts=walltime+logical-24 22.6ns ± 0% 18.8ns ± 0% -16.53% (p=0.000 n=9+8) EncodeMVCCKey/key=short/ts=all-24 22.6ns ± 0% 19.1ns ± 0% -15.32% (p=0.000 n=10+10) EncodeMVCCKey/key=long/ts=empty-24 65.2ns ± 1% 58.6ns ± 0% -10.11% (p=0.000 n=9+9) EncodeMVCCKey/key=long/ts=walltime-24 62.6ns ± 0% 59.6ns ± 1% -4.72% (p=0.000 n=10+10) EncodeMVCCKey/key=long/ts=walltime+logical-24 63.5ns ± 0% 60.2ns ± 0% -5.16% (p=0.000 n=10+10) EncodeMVCCKey/key=long/ts=all-24 63.9ns ± 0% 59.6ns ± 0% -6.81% (p=0.000 n=9+9) ``` It also yields a nice improvement over 22.1 with Go 1.18: ``` name old time/op new time/op delta EncodeMVCCKey/key=empty/ts=empty-24 15.7ns ± 0% 15.7ns ± 5% ~ (p=0.497 n=8+10) EncodeMVCCKey/key=empty/ts=walltime-24 19.5ns ± 0% 17.8ns ± 0% -8.99% (p=0.000 n=10+10) EncodeMVCCKey/key=empty/ts=walltime+logical-24 20.2ns ± 0% 18.6ns ± 0% -8.03% (p=0.000 n=10+10) EncodeMVCCKey/key=empty/ts=all-24 20.4ns ± 0% 18.8ns ± 0% -7.67% (p=0.000 n=9+9) EncodeMVCCKey/key=short/ts=empty-24 16.0ns ± 0% 15.8ns ± 0% -1.47% (p=0.000 n=10+10) EncodeMVCCKey/key=short/ts=walltime-24 20.6ns ± 0% 18.0ns ± 0% -12.51% (p=0.000 n=8+8) EncodeMVCCKey/key=short/ts=walltime+logical-24 20.9ns ± 0% 18.8ns ± 0% -9.82% (p=0.000 n=10+8) EncodeMVCCKey/key=short/ts=all-24 20.9ns ± 0% 19.1ns ± 0% -8.79% (p=0.000 n=9+10) EncodeMVCCKey/key=long/ts=walltime-24 60.4ns ± 0% 59.6ns ± 1% -1.30% (p=0.000 n=9+10) EncodeMVCCKey/key=long/ts=walltime+logical-24 61.4ns ± 0% 60.2ns ± 0% -1.84% (p=0.000 n=10+10) EncodeMVCCKey/key=long/ts=all-24 61.4ns ± 0% 59.6ns ± 0% -2.88% (p=0.000 n=10+9) EncodeMVCCKey/key=long/ts=empty-24 59.1ns ± 0% 58.6ns ± 0% -0.77% (p=0.000 n=10+9) ``` Resolves #88818. Release note: None Co-authored-by: Oleg Afanasyev <[email protected]> Co-authored-by: Erik Grinaker <[email protected]>
cockroachdb · Sep 28, 2022 · e783f14 · e783f14
3 parents 8521da7 + e6a121b + fecf978
commit e783f14
Show file tree

Hide file tree

Showing 3 changed files with 72 additions and 31 deletions.
diff --git a/pkg/cmd/roachtest/tests/clearrange.go b/pkg/cmd/roachtest/tests/clearrange.go
@@ -27,44 +27,63 @@ import (
 
 func registerClearRange(r registry.Registry) {
 	for _, checks := range []bool{true, false} {
-		checks := checks
-		r.Add(registry.TestSpec{
-			Name:  fmt.Sprintf(`clearrange/checks=%t`, checks),
-			Owner: registry.OwnerStorage,
-			// 5h for import, 90 for the test. The import should take closer
-			// to <3:30h but it varies.
-			Timeout: 5*time.Hour + 90*time.Minute,
-			Cluster: r.MakeClusterSpec(10, spec.CPU(16)),
-			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-				runClearRange(ctx, t, c, checks)
-			},
-		})
-
-		// Using a separate clearrange test on zfs instead of randomly
-		// using the same test, cause the Timeout might be different,
-		// and may need to be tweaked.
-		r.Add(registry.TestSpec{
-			Name:  fmt.Sprintf(`clearrange/zfs/checks=%t`, checks),
-			Skip:  "Consistently failing. See #68716 context.",
-			Owner: registry.OwnerStorage,
-			// 5h for import, 120 for the test. The import should take closer
-			// to <3:30h but it varies.
-			Timeout:           5*time.Hour + 120*time.Minute,
-			Cluster:           r.MakeClusterSpec(10, spec.CPU(16), spec.SetFileSystem(spec.Zfs)),
-			EncryptionSupport: registry.EncryptionMetamorphic,
-			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-				runClearRange(ctx, t, c, checks)
-			},
-		})
+		for _, rangeTombstones := range []bool{true, false} {
+			checks := checks
+			rangeTombstones := rangeTombstones
+			r.Add(registry.TestSpec{
+				Name:  fmt.Sprintf(`clearrange/checks=%t/rangeTs=%t`, checks, rangeTombstones),
+				Owner: registry.OwnerStorage,
+				// 5h for import, 90 for the test. The import should take closer
+				// to <3:30h but it varies.
+				Timeout: 5*time.Hour + 90*time.Minute,
+				Cluster: r.MakeClusterSpec(10, spec.CPU(16)),
+				Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
+					runClearRange(ctx, t, c, checks, rangeTombstones)
+				},
+			})
+
+			// Using a separate clearrange test on zfs instead of randomly
+			// using the same test, cause the Timeout might be different,
+			// and may need to be tweaked.
+			r.Add(registry.TestSpec{
+				Name:  fmt.Sprintf(`clearrange/zfs/checks=%t/rangeTs=%t`, checks, rangeTombstones),
+				Skip:  "Consistently failing. See #68716 context.",
+				Owner: registry.OwnerStorage,
+				// 5h for import, 120 for the test. The import should take closer
+				// to <3:30h but it varies.
+				Timeout:           5*time.Hour + 120*time.Minute,
+				Cluster:           r.MakeClusterSpec(10, spec.CPU(16), spec.SetFileSystem(spec.Zfs)),
+				EncryptionSupport: registry.EncryptionMetamorphic,
+				Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
+					runClearRange(ctx, t, c, checks, rangeTombstones)
+				},
+			})
+		}
 	}
 }
 
-func runClearRange(ctx context.Context, t test.Test, c cluster.Cluster, aggressiveChecks bool) {
+func runClearRange(
+	ctx context.Context,
+	t test.Test,
+	c cluster.Cluster,
+	aggressiveChecks bool,
+	useRangeTombstones bool,
+) {
 	c.Put(ctx, t.Cockroach(), "./cockroach")
 
 	t.Status("restoring fixture")
 	c.Start(ctx, t.L(), option.DefaultStartOpts(), install.MakeClusterSettings())
 
+	{
+		conn := c.Conn(ctx, t.L(), 1)
+		if _, err := conn.ExecContext(ctx,
+			`SET CLUSTER SETTING storage.mvcc.range_tombstones.enabled = $1`,
+			useRangeTombstones); err != nil {
+			t.Fatal(err)
+		}
+		conn.Close()
+	}
+
 	// NB: on a 10 node cluster, this should take well below 3h.
 	tBegin := timeutil.Now()
 	c.Run(ctx, c.Node(1), "./cockroach", "workload", "fixtures", "import", "bank",

diff --git a/pkg/util/hlc/timestamp.go b/pkg/util/hlc/timestamp.go
@@ -194,11 +194,13 @@ func (t Timestamp) AsOfSystemTime() string {
 }
 
 // IsEmpty returns true if t is an empty Timestamp.
+// gcassert:inline
 func (t Timestamp) IsEmpty() bool {
-	return t == Timestamp{}
+	return t.WallTime == 0 && t.Logical == 0 && !t.Synthetic
 }
 
 // IsSet returns true if t is not an empty Timestamp.
+// gcassert:inline
 func (t Timestamp) IsSet() bool {
 	return !t.IsEmpty()
 }

diff --git a/pkg/util/hlc/timestamp_test.go b/pkg/util/hlc/timestamp_test.go
@@ -405,3 +405,23 @@ func BenchmarkTimestampStringSynthetic(b *testing.B) {
 		_ = ts.String()
 	}
 }
+
+func BenchmarkTimestampIsEmpty(b *testing.B) {
+	cases := map[string]Timestamp{
+		"empty":    {},
+		"walltime": {WallTime: 1664364012528805328},
+		"all":      {WallTime: 1664364012528805328, Logical: 65535, Synthetic: true},
+	}
+
+	var result bool
+
+	for name, ts := range cases {
+		b.Run(name, func(b *testing.B) {
+			for i := 0; i < b.N; i++ {
+				result = ts.IsEmpty()
+			}
+		})
+	}
+
+	_ = result // make sure compiler doesn't optimize away the call
+}