Skip to content

Commit d34c5aa

Browse files
stevenschlanskerClaude (on behalf of Steven Schlansker)
andauthored
perf(format): cache compact row layout per nested slot (#3717)
## Why? Avoid redundant re-computation of compact codec offsets layout, saving cpu and memory allocations and fix(format): honor input offset when copying inline-struct bytes ## What does this PR do? perf(format): cache compact row layout per nested slot Hoist offsets, widths, and nullability into CompactRowLayout, built once per encoder. getStruct does no per-call lookup; inline-struct reads go directly to the parent buffer; the writer reads schema- derived state from the cache; compact rows skip the unused extData[] allocation. For List<Struct> fields the parent layout also carries the element layout so getArray(i) does not rebuild it. Fix a latent inline-width bug in CompactBinaryRowWriter that was masked by the prior slicing getStruct: writeUnaligned and writeAlignedBytes dropped the input offset, so re-emitting a child row via BinaryWriter.write(int, BinaryRow) copied the wrong bytes. ## Related issues ## AI Contribution Checklist - [x] Substantial AI assistance was used in this PR: `yes` / `no` AI Usage Disclosure - substantial_ai_assistance: yes - scope: all - affected_files_or_subsystems: Java row format - ai_review: ✔️ - ai_review_artifacts: > One commit, perf(format): cache compact row layout per nested slot. Caches fixedOffsets, per-field fixedWidths, allFieldsNotNullable, and bitmapWidthInBytes in a new CompactRowLayout value, computed once per array (element type is fixed) or memoized per parent extData slot. > The getStruct hot path previously rebuilt fixedOffsets and re-walked allNotNullable on every call. > Recommendation: Approve. Numbers in the commit message match a real bottleneck (the int[] + allNotNullable walk per nested decode), the implementation preserves the existing wire behavior and ownership model, no public API regression, all format tests pass, and the new caching follows the existing extData-slot pattern rather than inventing a new mechanism. > — Reviewed by Claude (Opus 4.7), 2026-05-29 > Skill used: I ran this through the fory-code-review skill, which delegated the diff inspection to a read-only review subagent loaded with the Fory review checklist, the lesson-derived red flags, and the Java runtime rules. > Verdict > No blockers. Recommend submission. Both commits are coherent and tested: > - d5cbdc2 is a real, narrow bugfix: the three put/copyFrom(..., 0, numBytes) sites in CompactBinaryRowWriter were copying from the start of the source buffer instead of from baseOffset. After schema sorting, nested inline structs land at non-zero offsets, so this corrupted writes for any non-trivial schema. CompactCodecTest.testCopyInlineStructViaWriter exercises that exact failure mode. Wire format unchanged — only the source byte range was wrong. > - 2b1825d precomputes a CompactRowLayout once per writer instead of recomputing field offsets and nested layouts on every write. Cache is per-CompactBinaryRowWriter instance, no map, no growth, no thread-safety surface (writers are already not thread-safe). CompactRowLayoutTest pins identity via assertSame at multiple nesting levels and covers the allFieldsNotNullable bitmap-skip path. CompactBinaryRow.getStruct also drops a per-call buffer.slice() allocation by pointing to the parent buffer at the right offset instead. - human_verification: ✔️ - performance_verification: ✔️ - provenance_license_confirmation: ✔️ ## Does this PR introduce any user-facing change? Compact codec user enjoys more CPU and memory on business logic without even realizing it. No public API or wire changes. ## Benchmark ``` RowFormatAllocationProbe bytes/op vs apache/main, compact mode, median of five: root 100208 -> 74305 (-25.9%) array 9032 -> 6629 (-26.6%) matrix 72005 -> 52760 (-26.7%) map 7744 -> 6401 (-17.3%) encode 55423 -> 41762 (-24.6%) Standard mode unchanged within noise. ``` --------- Co-authored-by: Claude (on behalf of Steven Schlansker) <stevenschlansker+claude@apache.org>
1 parent 7e52de7 commit d34c5aa

16 files changed

Lines changed: 433 additions & 130 deletions

java/fory-format/src/main/java/org/apache/fory/format/encoder/BinaryRowEncoder.java

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929

3030
class BinaryRowEncoder<T> implements RowEncoder<T> {
3131
private final Schema schema;
32-
private final Encoding codecFactory;
3332
private final GeneratedRowEncoder codec;
3433
private final BaseBinaryRowWriter writer;
3534
private final boolean sizeEmbedded;
@@ -38,12 +37,10 @@ class BinaryRowEncoder<T> implements RowEncoder<T> {
3837

3938
BinaryRowEncoder(
4039
final Schema schema,
41-
final Encoding codecFactory,
4240
final GeneratedRowEncoder codec,
4341
final BaseBinaryRowWriter writer,
4442
final boolean sizeEmbedded) {
4543
this.schema = schema;
46-
this.codecFactory = codecFactory;
4744
this.codec = codec;
4845
this.writer = writer;
4946
this.sizeEmbedded = sizeEmbedded;
@@ -82,7 +79,7 @@ T decode(final MemoryBuffer buffer, final int size) {
8279
schema, schemaHash, peerSchemaHash));
8380
}
8481
final int rowSize = size - 8;
85-
final BinaryRow row = codecFactory.newRow(schema);
82+
final BinaryRow row = writer.newRow();
8683
row.pointTo(buffer, buffer.readerIndex(), rowSize);
8784
buffer.increaseReaderIndex(rowSize);
8885
return fromRow(row);

java/fory-format/src/main/java/org/apache/fory/format/encoder/CompactCodecFormat.java

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,8 @@
2323
import java.util.Map;
2424
import org.apache.fory.format.row.binary.BinaryArray;
2525
import org.apache.fory.format.row.binary.BinaryMap;
26-
import org.apache.fory.format.row.binary.BinaryRow;
2726
import org.apache.fory.format.row.binary.CompactBinaryArray;
2827
import org.apache.fory.format.row.binary.CompactBinaryMap;
29-
import org.apache.fory.format.row.binary.CompactBinaryRow;
3028
import org.apache.fory.format.row.binary.writer.BaseBinaryRowWriter;
3129
import org.apache.fory.format.row.binary.writer.BinaryArrayWriter;
3230
import org.apache.fory.format.row.binary.writer.CompactBinaryArrayWriter;
@@ -76,11 +74,6 @@ public MapEncoderBuilder newMapEncoder(
7674
return new CompactMapEncoderBuilder(mapType, beanToken);
7775
}
7876

79-
@Override
80-
public BinaryRow newRow(final Schema schema) {
81-
return new CompactBinaryRow(schema);
82-
}
83-
8477
@Override
8578
public BinaryArray newArray(final Field field) {
8679
return new CompactBinaryArray(field);

java/fory-format/src/main/java/org/apache/fory/format/encoder/DefaultCodecFormat.java

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import java.util.Map;
2424
import org.apache.fory.format.row.binary.BinaryArray;
2525
import org.apache.fory.format.row.binary.BinaryMap;
26-
import org.apache.fory.format.row.binary.BinaryRow;
2726
import org.apache.fory.format.row.binary.writer.BaseBinaryRowWriter;
2827
import org.apache.fory.format.row.binary.writer.BinaryArrayWriter;
2928
import org.apache.fory.format.row.binary.writer.BinaryRowWriter;
@@ -72,11 +71,6 @@ public MapEncoderBuilder newMapEncoder(
7271
return new MapEncoderBuilder(mapType, beanToken);
7372
}
7473

75-
@Override
76-
public BinaryRow newRow(final Schema schema) {
77-
return new BinaryRow(schema);
78-
}
79-
8074
@Override
8175
public BinaryArray newArray(final Field field) {
8276
return new BinaryArray(field);

java/fory-format/src/main/java/org/apache/fory/format/encoder/Encoding.java

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import java.util.Map;
2424
import org.apache.fory.format.row.binary.BinaryArray;
2525
import org.apache.fory.format.row.binary.BinaryMap;
26-
import org.apache.fory.format.row.binary.BinaryRow;
2726
import org.apache.fory.format.row.binary.writer.BaseBinaryRowWriter;
2827
import org.apache.fory.format.row.binary.writer.BinaryArrayWriter;
2928
import org.apache.fory.format.type.Field;
@@ -47,8 +46,6 @@ ArrayEncoderBuilder newArrayEncoder(
4746

4847
MapEncoderBuilder newMapEncoder(TypeRef<? extends Map<?, ?>> mapType, TypeRef<?> beanToken);
4948

50-
BinaryRow newRow(Schema schema);
51-
5249
BinaryArray newArray(Field field);
5350

5451
BinaryMap newMap(Field field);

java/fory-format/src/main/java/org/apache/fory/format/encoder/RowCodecBuilder.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ Function<BaseBinaryRowWriter, RowEncoder<T>> buildForWriter() {
6363
@Override
6464
public RowEncoder<T> apply(final BaseBinaryRowWriter writer) {
6565
return new BinaryRowEncoder<T>(
66-
schema, codecFormat, rowEncoderFactory.apply(writer), writer, sizeEmbedded);
66+
schema, rowEncoderFactory.apply(writer), writer, sizeEmbedded);
6767
}
6868
};
6969
}

java/fory-format/src/main/java/org/apache/fory/format/row/binary/BinaryArray.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,13 @@ public class BinaryArray extends UnsafeTrait implements ArrayData {
5858

5959
public BinaryArray(Field field) {
6060
this(field, elementSize(field));
61+
initializeExtData(1); // Only require at most one slot to cache the schema for array type.
6162
}
6263

64+
/** Skips {@code extData}; subclass must override {@code getStruct(int, Field, int)}. */
6365
protected BinaryArray(Field field, int elementSize) {
6466
this.field = field;
6567
this.elementSize = elementSize;
66-
initializeExtData(1); // Only require at most one slot to cache the schema for array type.
6768
}
6869

6970
private static int elementSize(Field field) {

java/fory-format/src/main/java/org/apache/fory/format/row/binary/BinaryRow.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,14 @@ public BinaryRow(Schema schema) {
7676
initializeExtData(numFields);
7777
}
7878

79+
/** Skips {@code extData}; subclass must override {@code getStruct(int, Field, int)}. */
80+
protected BinaryRow(Schema schema, int bitmapWidthInBytes) {
81+
this.schema = schema;
82+
this.numFields = schema.numFields();
83+
Preconditions.checkArgument(numFields > 0);
84+
this.bitmapWidthInBytes = bitmapWidthInBytes;
85+
}
86+
7987
protected int computeBitmapWidthInBytes() {
8088
return BitUtils.calculateBitmapWidthInBytes(numFields);
8189
}

java/fory-format/src/main/java/org/apache/fory/format/row/binary/CompactBinaryArray.java

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
import org.apache.fory.format.row.binary.writer.CompactBinaryRowWriter;
2525
import org.apache.fory.format.type.DataTypes;
2626
import org.apache.fory.format.type.Field;
27-
import org.apache.fory.format.type.Schema;
2827
import org.apache.fory.memory.MemoryBuffer;
2928

3029
public class CompactBinaryArray extends BinaryArray {
@@ -33,12 +32,30 @@ public class CompactBinaryArray extends BinaryArray {
3332
private final boolean elementNullable;
3433
private int headerInBytes;
3534

35+
/** {@code null} when the element is not a struct. */
36+
private final CompactRowLayout elementLayout;
37+
38+
/** Cold path. Hot reads use {@link #CompactBinaryArray(Field, CompactRowLayout)}. */
3639
public CompactBinaryArray(final Field field) {
40+
this(field, null);
41+
}
42+
43+
/** {@code prebuiltElementLayout} may be {@code null}; built on demand for struct elements. */
44+
CompactBinaryArray(final Field field, final CompactRowLayout prebuiltElementLayout) {
3745
super(field, CompactBinaryArrayWriter.elementWidth(field));
3846
DataTypes.ListType listType = (DataTypes.ListType) field.type();
3947
elementField = listType.valueField();
4048
fixedWidth = CompactBinaryRowWriter.fixedWidthFor(elementField);
4149
elementNullable = elementField.nullable();
50+
if (elementField.type() instanceof DataTypes.StructType) {
51+
elementLayout =
52+
prebuiltElementLayout != null
53+
? prebuiltElementLayout
54+
: new CompactRowLayout(
55+
CompactBinaryRowWriter.sortSchema(DataTypes.createSchema(elementField)));
56+
} else {
57+
elementLayout = null;
58+
}
4259
}
4360

4461
@Override
@@ -105,28 +122,18 @@ protected BinaryRow getStruct(final int ordinal, final Field field, final int ex
105122
return null;
106123
}
107124
assert field == elementField;
125+
final CompactBinaryRow row = elementLayout.newRow();
108126
if (fixedWidth == -1) {
109-
return super.getStruct(ordinal, field, extDataSlot);
110-
}
111-
if (extData[extDataSlot] == null) {
112-
extData[extDataSlot] = newSchema(field);
127+
final long offsetAndSize = getInt64(ordinal);
128+
final int relativeOffset = (int) (offsetAndSize >> 32);
129+
final int size = (int) offsetAndSize;
130+
row.pointTo(getBuffer(), getBaseOffset() + relativeOffset, size);
131+
} else {
132+
row.pointTo(getBuffer(), getOffset(ordinal), fixedWidth);
113133
}
114-
final BinaryRow row = newRow((Schema) extData[extDataSlot]);
115-
row.pointTo(getBuffer(), getOffset(ordinal), fixedWidth);
116134
return row;
117135
}
118136

119-
@Override
120-
protected Schema newSchema(final Field field) {
121-
return CompactBinaryRowWriter.sortSchema(super.newSchema(field));
122-
}
123-
124-
@Override
125-
protected BinaryRow newRow(final Schema schema) {
126-
// TODO: don't re-compute fixed offsets
127-
return new CompactBinaryRow(schema, CompactBinaryRowWriter.fixedOffsets(schema));
128-
}
129-
130137
@Override
131138
protected BinaryArray newArray(final Field field) {
132139
return new CompactBinaryArray(field);

java/fory-format/src/main/java/org/apache/fory/format/row/binary/CompactBinaryRow.java

Lines changed: 31 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,6 @@
1919

2020
package org.apache.fory.format.row.binary;
2121

22-
import org.apache.fory.format.row.binary.writer.CompactBinaryRowWriter;
23-
import org.apache.fory.format.type.DataTypes;
2422
import org.apache.fory.format.type.Field;
2523
import org.apache.fory.format.type.Schema;
2624
import org.apache.fory.memory.MemoryBuffer;
@@ -41,47 +39,39 @@
4139
* <b>The compact format is still under development and may not be stable yet.</b>
4240
*/
4341
public class CompactBinaryRow extends BinaryRow {
44-
private final boolean allFieldsNotNullable;
45-
private final int[] fixedOffsets;
42+
private final CompactRowLayout layout;
4643
private final int bitmapOffset;
4744

4845
public CompactBinaryRow(final Schema schema) {
49-
this(schema, CompactBinaryRowWriter.fixedOffsets(schema));
46+
this(new CompactRowLayout(schema));
5047
}
5148

52-
public CompactBinaryRow(final Schema schema, final int[] fixedOffsets) {
53-
super(schema);
54-
this.fixedOffsets = fixedOffsets;
55-
bitmapOffset = fixedOffsets[fixedOffsets.length - 1];
56-
allFieldsNotNullable = CompactBinaryRowWriter.allNotNullable(schema.fields());
49+
CompactBinaryRow(CompactRowLayout layout) {
50+
super(layout.schema, layout.bitmapWidthInBytes);
51+
this.layout = layout;
52+
this.bitmapOffset = layout.fixedOffsets[layout.fixedOffsets.length - 1];
5753
}
5854

59-
@Override
60-
protected int computeBitmapWidthInBytes() {
61-
// cannot use field due to initialization order
62-
if (CompactBinaryRowWriter.allNotNullable(schema.fields())) {
63-
return 0;
64-
}
65-
return CompactBinaryRowWriter.headerBytes(schema);
55+
CompactRowLayout getLayout() {
56+
return layout;
6657
}
6758

6859
@Override
6960
public boolean isNullAt(final int ordinal) {
70-
if (allFieldsNotNullable) {
61+
if (layout.allFieldsNotNullable) {
7162
return false;
7263
}
7364
return super.isNullAt(ordinal);
7465
}
7566

76-
// TODO: this should use StableValue once it's available
7767
@Override
7868
public int getOffset(final int ordinal) {
79-
return baseOffset + fixedOffsets[ordinal];
69+
return baseOffset + layout.fixedOffsets[ordinal];
8070
}
8171

8272
@Override
8373
public MemoryBuffer getBuffer(final int ordinal) {
84-
final int fixedWidthBinary = CompactBinaryRowWriter.fixedWidthFor(schema, ordinal);
74+
final int fixedWidthBinary = layout.fixedWidths[ordinal];
8575
if (fixedWidthBinary >= 0) {
8676
if (isNullAt(ordinal)) {
8777
return null;
@@ -94,7 +84,7 @@ public MemoryBuffer getBuffer(final int ordinal) {
9484

9585
@Override
9686
public byte[] getBinary(final int ordinal) {
97-
final int fixedWidthBinary = CompactBinaryRowWriter.fixedWidthFor(schema, ordinal);
87+
final int fixedWidthBinary = layout.fixedWidths[ordinal];
9888
if (fixedWidthBinary >= 0) {
9989
if (isNullAt(ordinal)) {
10090
return null;
@@ -112,27 +102,30 @@ protected BinaryRow getStruct(final int ordinal, final Field field, final int ex
112102
if (isNullAt(ordinal)) {
113103
return null;
114104
}
115-
final int fixedWidthBinary = CompactBinaryRowWriter.fixedWidthFor(schema, ordinal);
105+
final CompactBinaryRow row = layout.childLayouts[ordinal].newRow();
106+
final int fixedWidthBinary = layout.fixedWidths[ordinal];
116107
if (fixedWidthBinary == -1) {
117-
return super.getStruct(ordinal, field, extDataSlot);
118-
}
119-
if (extData[extDataSlot] == null) {
120-
extData[extDataSlot] = DataTypes.createSchema(field);
108+
final long offsetAndSize = getInt64(ordinal);
109+
final int relativeOffset = (int) (offsetAndSize >> 32);
110+
final int size = (int) offsetAndSize;
111+
row.pointTo(getBuffer(), getBaseOffset() + relativeOffset, size);
112+
} else {
113+
row.pointTo(getBuffer(), getOffset(ordinal), fixedWidthBinary);
121114
}
122-
final BinaryRow row = newRow((Schema) extData[extDataSlot]);
123-
row.pointTo(getBuffer().slice(getOffset(ordinal), fixedWidthBinary), 0, fixedWidthBinary);
124115
return row;
125116
}
126117

127118
@Override
128-
protected Schema newSchema(final Field field) {
129-
return CompactBinaryRowWriter.sortSchema(super.newSchema(field));
130-
}
131-
132-
@Override
133-
protected BinaryRow newRow(final Schema schema) {
134-
// TODO: avoid re-computing these offsets
135-
return new CompactBinaryRow(schema, CompactBinaryRowWriter.fixedOffsets(schema));
119+
BinaryArray getArray(final int ordinal, final Field field) {
120+
if (isNullAt(ordinal)) {
121+
return null;
122+
}
123+
final long offsetAndSize = getInt64(ordinal);
124+
final int relativeOffset = (int) (offsetAndSize >> 32);
125+
final int size = (int) offsetAndSize;
126+
final CompactBinaryArray array = new CompactBinaryArray(field, layout.childLayouts[ordinal]);
127+
array.pointTo(getBuffer(), getBaseOffset() + relativeOffset, size);
128+
return array;
136129
}
137130

138131
@Override
@@ -152,6 +145,6 @@ protected int nullBitmapOffset() {
152145

153146
@Override
154147
protected BinaryRow rowForCopy() {
155-
return new CompactBinaryRow(schema, fixedOffsets);
148+
return new CompactBinaryRow(layout);
156149
}
157150
}

0 commit comments

Comments
 (0)