path_compiler: make the hint bound proportional, not a constant

The constant ceiling in the previous commit regressed valid deep paths. I
benchmarked it rather than assuming, on a well-formed 2000-component path:

  upstream, no clamp        32781 B/op   1 alloc/op
  constant 512 ceiling     113177 B/op   5 allocs/op    <- 3.45x worse
  proportional len/2+1      32788 B/op   1 alloc/op

A 512-element ceiling under-reserves any path with more components than that, so
append regrows repeatedly and the common case pays for the hostile one. Deep
paths are unusual but they are legal, and a defensive bound should not make valid
input worse.

The shortest component that can contribute a separator is two bytes ("k."), so
len(jsonPath)/2+1 can never under-reserve a well-formed path, while still
refusing to size the allocation from a long run of separators. The hostile case
is unaffected by the change:

  200000 separators, rejected on the first component:
  upstream 1606185 B/op -> 803352 B/op

The existing correctness test could not catch this: it passed with the constant
too, because clamping never changes what ParsePath returns. So the property now
has an allocation assertion of its own,
TestParsePathHintDoesNotRegressDeepPaths, which fails on the constant version
with "used 5 allocations, want 1" and passes here.

Verified that ./... behaves identically to unpatched upstream: the two
TestOracleSetPr286Regression subtests fail on a clean checkout as well, so they
are pre-existing and unrelated to this change.

Change-Id: I43e5ed1e205c70fb55dde128a4ab817e3d359d23

Author: tzh476

path_compiler.go

@@ -11,15 +11,21 @@ var (
 	errUnterminatedKey = errors.New("jsonparser: unterminated quoted key")
 )
 
-// maxPathHint bounds the capacity pre-allocated by ParsePath. Real paths have a
-// handful of components; the limit only stops an unvalidated path from sizing
-// the initial allocation.
-const maxPathHint = 512
-
-// pathHintCap clamps a pre-allocation hint to maxPathHint.
-func pathHintCap(n int) int {
-	if n > maxPathHint {
-		return maxPathHint
+// pathHintCap bounds the capacity pre-allocated by ParsePath.
+//
+// The bound is proportional to the path length rather than a constant. A
+// constant ceiling regresses legitimate deep paths: benchmarked on a valid
+// 2000-component path, a 512 ceiling took allocation from 32781 B in 1 alloc to
+// 113177 B in 5 allocs, because append then has to regrow. Deep paths are
+// unusual but they are legal, and a defensive bound should not make the valid
+// case worse.
+//
+// The shortest component that can contribute a separator is two bytes ("k."),
+// so len/2+1 can never under-reserve a well-formed path, while still refusing
+// to size the allocation from a long run of separators.
+func pathHintCap(n, pathLen int) int {
+	if max := pathLen/2 + 1; n > max {
+		return max
 	}
 	return n
 }
@@ -56,7 +62,7 @@ func ParsePath(jsonPath string) ([]string, error) {
 	// would otherwise reserve memory proportional to its length and then be
 	// rejected on the first component. Capacity is only a hint to append, which
 	// still grows as needed, so clamping cannot change the parsed result.
-	parts := make([]string, 0, pathHintCap(1+strings.Count(jsonPath, ".")+strings.Count(jsonPath, "[")))
+	parts := make([]string, 0, pathHintCap(1+strings.Count(jsonPath, ".")+strings.Count(jsonPath, "["), len(jsonPath)))
 
 	for pos := 0; pos < len(jsonPath); {
 		switch jsonPath[pos] {

path_compiler_clamp_test.go

@@ -51,3 +51,35 @@ func TestParsePathHintClampPreservesResults(t *testing.T) {
 		}
 	}
 }
+
+// TestParsePathHintDoesNotRegressDeepPaths pins the reason the bound is
+// proportional to the path length instead of a constant.
+//
+// A constant ceiling silently penalises valid deep paths: with a 512 ceiling a
+// well-formed 2000-component path allocated 113177 B across 5 allocations,
+// against 32781 B in 1 allocation unclamped, because append has to regrow. The
+// correctness test above cannot see that -- it passed with the constant too --
+// so the property needs an allocation assertion of its own.
+//
+// One allocation is the whole point: the hint has to be large enough that
+// append never regrows for a path that really does have this many components.
+func TestParsePathHintDoesNotRegressDeepPaths(t *testing.T) {
+	const n = 2000
+	keys := make([]string, n)
+	for i := range keys {
+		keys[i] = "k"
+	}
+	path := strings.Join(keys, ".")
+
+	var got []string
+	allocs := testing.AllocsPerRun(50, func() {
+		got, _ = ParsePath(path)
+	})
+	if len(got) != n {
+		t.Fatalf("got %d components, want %d", len(got), n)
+	}
+	if allocs > 1 {
+		t.Errorf("ParsePath on a valid %d-component path used %.0f allocations, want 1; "+
+			"the pre-allocation hint is under-reserving and append is regrowing", n, allocs)
+	}
+}