Skip to content

Commit 6124cb0

Browse files
dmitriplotnikovcopybara-github
authored andcommitted
Avoid intermediate slice views when reading token text
Lexing and parsing turn code point ranges into strings constantly: once for every identifier, keyword and literal. Both call sites spelled this as slice(i, j).toString(), which allocates an intermediate CelCodePointArray view solely to copy out of it and then discard it. Add CelCodePointArray.substring(i, j), which builds the String straight from the backing array, and implement it in each of the four subclasses. toString() becomes final and delegates to substring(0, size()), so the subclasses lose their near-duplicate toString() overrides. Lexer.consumeIdent and PrattParser.getTokenText call the new method. This removes exactly one 32-byte object per token whose text is materialized. Measured with CelParserBenchmark (parseOnly, built -c opt), comparing three parsers back to back in one session: ANTLR, the Pratt parser before this change, and the Pratt parser after it. Objects allocated per parse: | Case | ANTLR | Pratt before | Pratt after | Pratt vs ANTLR | Delta this CL | | :--- | ---: | ---: | ---: | ---: | ---: | | SMOKE_TEST | 357 | 123 | 120 | 3.0x smaller | -2.4% | | CHAINED_ORS | 968 | 349 | 339 | 2.9x smaller | -2.9% | | LIST_COMPREHENSION | 512 | 166 | 160 | 3.2x smaller | -3.6% | | MESSAGE_CREATION | 1,253 | 426 | 406 | 3.1x smaller | -4.7% | | LONG_LIST | 81,794 | 19,265 | 18,263 | 4.5x smaller | -5.2% | Bytes allocated per parse: | Case | ANTLR | Pratt before | Pratt after | Pratt vs ANTLR | Delta this CL | | :--- | ---: | ---: | ---: | ---: | ---: | | SMOKE_TEST | 12,256 | 3,608 | 3,512 | 3.5x smaller | -2.7% | | CHAINED_ORS | 32,160 | 9,912 | 9,592 | 3.4x smaller | -3.2% | | LIST_COMPREHENSION | 17,320 | 4,928 | 4,736 | 3.7x smaller | -3.9% | | MESSAGE_CREATION | 43,128 | 13,056 | 12,416 | 3.5x smaller | -4.9% | | LONG_LIST | 2,907,488 | 563,952 | 531,888 | 5.5x smaller | -5.7% | Wall clock, mean of 3 caliper trial medians: | Case | ANTLR | Pratt before | Pratt after | Pratt vs ANTLR | Delta this CL | | :--- | ---: | ---: | ---: | ---: | ---: | | SMOKE_TEST | 4,940 ns | 696 ns | 684 ns | 7.2x faster | -1.8% | | CHAINED_ORS | 14,641 ns | 2,008 ns | 2,006 ns | 7.3x faster | -0.1% | | LIST_COMPREHENSION | 7,514 ns | 1,250 ns | 1,217 ns | 6.2x faster | -2.7% | | MESSAGE_CREATION | 20,979 ns | 3,526 ns | 3,514 ns | 6.0x faster | -0.4% | | LONG_LIST | 1,616,631 ns | 140,500 ns | 146,640 ns | 11.0x faster | +4.4% | Wall clock is unchanged within measurement noise. The per-case deltas run from -2.7% to +4.4% and straddle zero, which is what a change that removes 3-5% of allocations and no actual work should look like. The LONG_LIST row reads as a regression, but that trial was noisy (per-trial medians 144.6us, 155.7us, 139.6us, against a much tighter 142.1us, 141.1us, 138.3us before) and its fastest observed parse, 134.6us, is below the 135.7us baseline. The win here is allocation volume and the GC pressure that follows from it. This is the first in a series of parser changes; the wall-clock improvements come later in that series. The ANTLR column is included for scale, and shows why the Pratt parser exists. ANTLR is slow enough on LONG_LIST that the case exceeds caliper's default 5 minute per-trial budget and has to be measured with a raised --time-limit. PiperOrigin-RevId: 981290051
1 parent 33da350 commit 6124cb0

8 files changed

Lines changed: 127 additions & 23 deletions

File tree

‎common/src/main/java/dev/cel/common/internal/BasicCodePointArray.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,13 +58,14 @@ public BasicCodePointArray slice(int i, int j) {
5858
}
5959

6060
@Override
61-
public int get(int index) {
62-
checkElementIndex(index, size());
63-
return codePoints()[offset() + index] & 0xffff;
61+
public String substring(int i, int j) {
62+
checkPositionIndexes(i, j, size());
63+
return new String(codePoints(), offset() + i, j - i);
6464
}
6565

6666
@Override
67-
public final String toString() {
68-
return new String(codePoints(), offset(), size());
67+
public int get(int index) {
68+
checkElementIndex(index, size());
69+
return codePoints()[offset() + index] & 0xffff;
6970
}
7071
}

‎common/src/main/java/dev/cel/common/internal/CelCodePointArray.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,14 @@ public abstract class CelCodePointArray {
3636
/** Returns a new {@link CelCodePointArray} that is a subview of this between [i, j). */
3737
public abstract CelCodePointArray slice(int i, int j);
3838

39+
/**
40+
* Returns the code points between [i, j) as a string.
41+
*
42+
* <p>Equivalent to {@code slice(i, j).toString()}, but does not materialize the intermediate
43+
* view. Lexing and parsing call this for every literal and identifier.
44+
*/
45+
public abstract String substring(int i, int j);
46+
3947
/** Get the code point at the given index. */
4048
public abstract int get(int index);
4149

@@ -55,7 +63,9 @@ public boolean isEmpty() {
5563
}
5664

5765
@Override
58-
public abstract String toString();
66+
public final String toString() {
67+
return substring(0, size());
68+
}
5969

6070
public static CelCodePointArray fromString(String text) {
6171
if (isNullOrEmpty(text)) {

‎common/src/main/java/dev/cel/common/internal/EmptyCodePointArray.java‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
package dev.cel.common.internal;
1616

17+
import static com.google.common.base.Preconditions.checkPositionIndexes;
18+
1719
import com.google.common.collect.ImmutableList;
1820
import com.google.errorprone.annotations.DoNotCall;
1921
import com.google.errorprone.annotations.Immutable;
@@ -51,6 +53,12 @@ public int get(int index) {
5153
String.format("index (%s) must not be greater than size (0)", index));
5254
}
5355

56+
@Override
57+
public String substring(int i, int j) {
58+
checkPositionIndexes(i, j, 0);
59+
return "";
60+
}
61+
5462
@Override
5563
public int size() {
5664
return 0;
@@ -60,9 +68,4 @@ public int size() {
6068
public ImmutableList<Integer> lineOffsets() {
6169
return ImmutableList.of(1);
6270
}
63-
64-
@Override
65-
public String toString() {
66-
return "";
67-
}
6871
}

‎common/src/main/java/dev/cel/common/internal/Latin1CodePointArray.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,13 +58,14 @@ public Latin1CodePointArray slice(int i, int j) {
5858
}
5959

6060
@Override
61-
public int get(int index) {
62-
checkElementIndex(index, size());
63-
return Byte.toUnsignedInt(codePoints()[offset() + index]);
61+
public String substring(int i, int j) {
62+
checkPositionIndexes(i, j, size());
63+
return new String(codePoints(), offset() + i, j - i, ISO_8859_1);
6464
}
6565

6666
@Override
67-
public final String toString() {
68-
return new String(codePoints(), offset(), size(), ISO_8859_1);
67+
public int get(int index) {
68+
checkElementIndex(index, size());
69+
return Byte.toUnsignedInt(codePoints()[offset() + index]);
6970
}
7071
}

‎common/src/main/java/dev/cel/common/internal/SupplementalCodePointArray.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -59,13 +59,14 @@ public SupplementalCodePointArray slice(int i, int j) {
5959
}
6060

6161
@Override
62-
public int get(int index) {
63-
checkElementIndex(index, size());
64-
return codePoints()[offset() + index];
62+
public String substring(int i, int j) {
63+
checkPositionIndexes(i, j, size());
64+
return new String(codePoints(), offset() + i, j - i);
6565
}
6666

6767
@Override
68-
public final String toString() {
69-
return new String(codePoints(), offset(), size());
68+
public int get(int index) {
69+
checkElementIndex(index, size());
70+
return codePoints()[offset() + index];
7071
}
7172
}

‎common/src/test/java/dev/cel/common/internal/CelCodePointArrayTest.java‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package dev.cel.common.internal;
1616

1717
import static com.google.common.truth.Truth.assertThat;
18+
import static org.junit.Assert.assertThrows;
1819

1920
import com.google.auto.value.AutoValue;
2021
import com.google.common.collect.ImmutableList;
@@ -39,6 +40,93 @@ public void computeLineOffset(
3940
.inOrder();
4041
}
4142

43+
@Test
44+
public void substring_empty() {
45+
CelCodePointArray empty = CelCodePointArray.fromString("");
46+
assertThat(empty).isInstanceOf(EmptyCodePointArray.class);
47+
48+
assertThat(empty.substring(0, 0)).isEmpty();
49+
assertThrows(IndexOutOfBoundsException.class, () -> empty.substring(0, 1));
50+
assertThrows(IndexOutOfBoundsException.class, () -> empty.substring(-1, 0));
51+
assertThrows(IndexOutOfBoundsException.class, () -> empty.substring(1, 0));
52+
assertThrows(IndexOutOfBoundsException.class, () -> empty.substring(1, 1));
53+
}
54+
55+
@Test
56+
public void substring_latin1() {
57+
CelCodePointArray latin1 = CelCodePointArray.fromString("hello world");
58+
assertThat(latin1).isInstanceOf(Latin1CodePointArray.class);
59+
60+
assertThat(latin1.substring(0, 5)).isEqualTo("hello");
61+
assertThat(latin1.substring(6, 11)).isEqualTo("world");
62+
assertThat(latin1.substring(0, 11)).isEqualTo("hello world");
63+
assertThat(latin1.substring(3, 3)).isEmpty();
64+
65+
assertThrows(IndexOutOfBoundsException.class, () -> latin1.substring(-1, 5));
66+
assertThrows(IndexOutOfBoundsException.class, () -> latin1.substring(0, 12));
67+
assertThrows(IndexOutOfBoundsException.class, () -> latin1.substring(5, 4));
68+
69+
// Test on a sliced subview to ensure bounds are checked against size(), not the backing buffer
70+
// length
71+
CelCodePointArray sliced = latin1.slice(1, 4); // "ell", size = 3, buffer length = 11
72+
assertThat(sliced.substring(0, 3)).isEqualTo("ell");
73+
assertThat(sliced.substring(1, 2)).isEqualTo("l");
74+
75+
assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(-1, 2));
76+
assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(0, 4));
77+
assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(2, 1));
78+
}
79+
80+
@Test
81+
public void substring_basic() {
82+
CelCodePointArray basic = CelCodePointArray.fromString("abc \uff20 def");
83+
assertThat(basic).isInstanceOf(BasicCodePointArray.class);
84+
85+
assertThat(basic.substring(0, 3)).isEqualTo("abc");
86+
assertThat(basic.substring(4, 5)).isEqualTo("\uff20");
87+
assertThat(basic.substring(6, 9)).isEqualTo("def");
88+
assertThat(basic.substring(0, 9)).isEqualTo("abc \uff20 def");
89+
assertThat(basic.substring(3, 3)).isEmpty();
90+
91+
assertThrows(IndexOutOfBoundsException.class, () -> basic.substring(-1, 5));
92+
assertThrows(IndexOutOfBoundsException.class, () -> basic.substring(0, 10));
93+
assertThrows(IndexOutOfBoundsException.class, () -> basic.substring(5, 4));
94+
95+
// Test on a sliced subview to ensure bounds are checked against size(), not the backing buffer
96+
// length
97+
CelCodePointArray sliced = basic.slice(1, 5); // "bc \uff20", size = 4, buffer length = 9
98+
assertThat(sliced.substring(0, 4)).isEqualTo("bc \uff20");
99+
100+
assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(-1, 2));
101+
assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(0, 5));
102+
assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(2, 1));
103+
}
104+
105+
@Test
106+
public void substring_supplemental() {
107+
CelCodePointArray supp = CelCodePointArray.fromString(" text 가나다 😦😁😑 ");
108+
assertThat(supp).isInstanceOf(SupplementalCodePointArray.class);
109+
110+
assertThat(supp.substring(0, 5)).isEqualTo(" text");
111+
assertThat(supp.substring(10, 13)).isEqualTo("😦😁😑");
112+
assertThat(supp.substring(0, supp.size())).isEqualTo(" text 가나다 😦😁😑 ");
113+
assertThat(supp.substring(3, 3)).isEmpty();
114+
115+
assertThrows(IndexOutOfBoundsException.class, () -> supp.substring(-1, 5));
116+
int greaterThanSize = supp.size() + 1;
117+
assertThrows(IndexOutOfBoundsException.class, () -> supp.substring(0, greaterThanSize));
118+
assertThrows(IndexOutOfBoundsException.class, () -> supp.substring(5, 4));
119+
120+
// Test on a sliced subview to ensure bounds are checked against size(), not the backing buffer
121+
// length
122+
CelCodePointArray sliced = supp.slice(1, 5); // "text", size = 4, buffer length = 15
123+
assertThat(sliced.substring(0, 4)).isEqualTo("text");
124+
125+
assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(-1, 2));
126+
assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(0, 5));
127+
assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(2, 1));
128+
}
129+
42130
@AutoValue
43131
abstract static class LineOffsetTestCase {
44132
abstract String text();

‎parser/src/main/java/dev/cel/parser/Lexer.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -635,7 +635,7 @@ private Token consumeIdent() {
635635
position++;
636636
}
637637
int end = position;
638-
String word = content.slice(start, end).toString();
638+
String word = content.substring(start, end);
639639
TokenType keywordType = KEYWORDS.get(word);
640640
if (keywordType != null) {
641641
return makeToken(keywordType, start, end);

‎parser/src/main/java/dev/cel/parser/PrattParser.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ private String getTokenText(Lexer.Token tok) {
198198
return tok.text;
199199
}
200200
if (tok.start >= 0 && tok.end >= tok.start && tok.end <= source.getContent().size()) {
201-
return source.getContent().slice(tok.start, tok.end).toString();
201+
return source.getContent().substring(tok.start, tok.end);
202202
}
203203
return "";
204204
}

0 commit comments

Comments
 (0)