Repository navigation
Conversation
|
Assigning reviewers: R: @Abacn for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Thanks, the analysis and the change make sense to me, and the benchmark looks promising. However since it's a core code path, adding contributor who had context here cc: @reuvenlax (for previous changes #5723 #14591) |
|
And @stankiewicz |
|
Will take a look. Can you provide more context on the advantages? In particular, JVM startup time is probably dominated by other things and JVM startup time is usually a tiny fraction of pipeline runtime. Are there other advantages here, or is it just startup time? |
|
Another comment: the benchmarks should also test large schemas (> 1000 fields), and these should have nested fields as well. There are users that have some very large schemas, and we don't want to regress them. |
|
Thanks! I will look a bit deeper and implement some benchmarks with large schemas/nested fields, it makes sense! Will keep this as draft for now. But yes, the advantage ends up being a much faster creation (for every schema), and way less bookkeeping of the generated classes, as JVM retains class metadata for everything instantiated. At the steady state, after it's classloaded and JIT has ran, it shouldn't make much difference. (Also I think it simplifies the code considerably, which might be a win by itself.) |
|
Reminder, please take a look at this pr: @Abacn |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @chamikaramj for label java. Available commands:
|
|
Reminder, please take a look at this pr: @chamikaramj @Abacn |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @kennknowles for label java. Available commands:
|
|
interesting! So we were doing code gen that didn't actually specialize the code to the purpose being generated for? This is definitely a nice simplification if all that codegen doesn't really do anything. I wonder if there are opportunities to make the codegen actually do what it is meant to do... be very specialized straight-line inline-method-call kind of code that just encodes exactly the thing it is supposed to. |
|
I want to be cautious here, since I'm not convinced that the codegen didn't
do anything. The savings here are all on worker startup time, which is
generally slow for other reason as well.
…On Wed, Sep 2, 2026 at 10:22 AM Kenn Knowles ***@***.***> wrote:
*kennknowles* left a comment (apache/beam#39619)
<#39619 (comment)>
interesting! So we were doing code gen that didn't actually specialize the
code to the purpose being generated for? This is definitely a nice
simplification if all that codegen doesn't really do anything. I wonder if
there are opportunities to make the codegen actually do what it is meant to
do... be very specialized straight-line inline-method-call kind of code
that just encodes exactly the thing it is supposed to.
—
Reply to this email directly, view it on GitHub
<#39619?email_source=notifications&email_token=AFAYJVJ2MMYNJWNKRWGGVOT5NBJOVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNJRGM2TKMBUGQ32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5513550447>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFAYJVKLLUY5RILKHJMPDQT5NBJOVAVCNFSNUABEKJSXA33TNF2G64TZHM2TAOJQGQZDINJ3JFZXG5LFHM2TANRVGEYTGMZXGKQXMAQ>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AFAYJVKLXCXDWANZDBWONVD5NBJOVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNJRGM2TKMBUGQ32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/AFAYJVOB5LORXO2UAFAI3KL5NBJOVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNJRGM2TKMBUGQ32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Thanks for pushing on this, @reuvenlax. I've added benchmarks with 1,024 top-level fields and nested rows, alongside the small schemas and both encoding modes. Each nested field contains a two-field row, so the largest case has 2,048 scalar leaves. The full results and commands are in the PR description. My earlier claim of equivalent steady-state performance was too strong. Nested dynamic decoding is about 1.5% slower, and a repeat with the implementation order reversed confirms that cost. The repeat also measures a 4.6% slowdown for the small nested static case. Large dynamic encoding is about 1% faster in the full matrix. Uncached coder creation is 1.9x to 36.7x faster across these shapes, but that does not establish a meaningful improvement in overall worker startup or pipeline runtime. Beyond creation latency, the change removes the per-schema generated classes and the machinery needed to create them. I have not measured the effect on a full worker. The JVM can optimize generated classes differently even when they call the same Java delegates, so the matching source-level loops alone do not settle the performance question. I've kept this as a draft and updated the description to make the measured decoding cost explicit. The new benchmark coverage is in 3326a88; the 47 targeted RowCoder and SchemaCoder tests pass. |
|
Seems that steady state performance penalty seems negligible but it doesn't seem like a no-brainer. There's the speedup in creation and code simplification but it doesn't come for free. So not sure if we should push for any of this. |
|
Reminder, please take a look at this pr: @kennknowles @Abacn |
|
Is there an opportunity to improve the code gen to make it more specialized? |
|
Reminder, please take a look at this pr: @kennknowles @Abacn |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @ahmedabu98 for label java. Available commands:
|
github-actions
Bot
added
Next Action: Author
and removed
slow-review
Next Action: Reviewers
labels
…rowcoder-bytecode-generation
da6c16d to
e9f36b3
Compare
…rowcoder-bytecode-generation
|
Yes, I think there is. The class generated today specializes very little: it holds the schema and A generator that really specializes could emit one call per field, with the component coder as a constant of its exact type and the row index hard-coded. It could also resolve the static-encoding option at generation time (today that is a map lookup per row). To size the opportunity, I wrote by hand what such a generator could emit for the two-field benchmark schemas and compared it with a copy of the loops. This code is not in the PR.
Change in ns/op of the straight-line code against the loop. Same JMH protocol as the PR description. Please read this as directional only. Other builds were running on the machine during both runs, the schemas have two fields and no nullable fields, and this is hand-written Java, not generated bytecode. The pattern does repeat across both runs: no clear gain for flat rows with dynamic encoding, about 5% to 14% for nested rows with dynamic encoding, and about 11% to 44% with static encoding. I also tried the cheap part without codegen: the existing loop, with the static-encoding option resolved once at construction. That gave 8% to 9% for static encoding on the encode side in one run and nothing clear elsewhere. So most of the difference seems to come from the per-field call sites. I did not profile to confirm that. Limits I would expect:
So the two directions conflict. This PR drops the generated class to make creation cheaper, and specialized codegen would spend more at creation to make steady state faster. It would be a separate change, built on the existing generator and not on this PR. Straight-line coder used for the flat schema (BYTE field0, BYTES field1)static final class FlatStraightCoder extends CustomCoder<Row> {
private static final ByteCoder FIELD0_CODER = ByteCoder.of();
private static final ByteArrayCoder FIELD1_CODER = ByteArrayCoder.of();
private final Schema schema;
private final boolean staticEncoding;
FlatStraightCoder(Schema schema) {
this.schema = schema;
this.staticEncoding =
schema.getOptions().getValueOrDefault("beam:option:row:static_encoding", false);
}
@Override
public void encode(Row value, OutputStream outputStream) throws IOException {
checkState(value.getFieldCount() == 2);
if (!staticEncoding) {
VAR_INT_CODER.encode(2, outputStream);
NULL_LIST_CODER.encode(EMPTY_BIT_SET, outputStream);
}
Byte field0 = value.getValue(0);
if (field0 != null) {
FIELD0_CODER.encode(field0, outputStream);
}
byte[] field1 = value.getValue(1);
if (field1 != null) {
FIELD1_CODER.encode(field1, outputStream);
}
}
@Override
public Row decode(InputStream inputStream) throws IOException {
Object[] fieldValues = new Object[2];
if (staticEncoding) {
fieldValues[0] = FIELD0_CODER.decode(inputStream);
fieldValues[1] = FIELD1_CODER.decode(inputStream);
} else {
// The field count and the null list still allow compatible schema changes.
int fieldCount = VAR_INT_CODER.decode(inputStream);
BitSet nullFields = NULL_LIST_CODER.decode(inputStream);
if (fieldCount > 0 && !nullFields.get(0)) {
fieldValues[0] = FIELD0_CODER.decode(inputStream);
}
if (fieldCount > 1 && !nullFields.get(1)) {
fieldValues[1] = FIELD1_CODER.decode(inputStream);
}
}
return Row.withSchema(schema).attachValues(fieldValues);
}
}The nested variant has the same shape, with two |
beam_PreCommit_Java failed on UnboundedSourceWrapperTest.testPollingOfIdleReaders in :runners:flink:1.19:test. The test uses StringUtf8Coder only and passes locally on this commit.
6a830f6 to
10f12d7
Compare
|
Interesting. Some comments inline:
On Thu, Oct 8, 2026 at 6:21 PM Bruno Volpato ***@***.***> wrote:
*bvolpato* left a comment (apache/beam#39619)
<#39619 (comment)>
Yes, I think there is. The class generated today specializes very little:
it holds the schema and hasNullableFields as constants, and encode and
decode forward to the same shared loops over Coder[] that this PR keeps.
Every schema goes through the same coders[i].encode(...) call sites.
A generator that really specializes could emit one call per field, with
the component coder as a constant of its exact type and the row index
hard-coded. It could also resolve the static-encoding option at generation
time (today that is a map lookup per row).
So basically you're suggesting unrolling the loop via generated code, right?
To size the opportunity, I wrote by hand what such a generator could emit
for the two-field benchmark schemas and compared it with a copy of the
loops. This code is not in the PR.
Operation Nested Static Run 1 Run 2
decode false false -0.3% -3.4%
decode false true -18.4% * -13.4% *
decode true false -6.2% -4.6%
decode true true -22.5% * -35.1% *
encode false false -3.0% -4.4%
encode false true -13.1% * -10.9% *
encode true false -14.1% -13.9% *
encode true true -43.5% * -31.4% *
Change in ns/op of the straight-line code against the loop. Same JMH
protocol as the PR description. * marks non-overlapping 99.9% intervals.
Please read this as directional only. Other builds were running on the
machine during both runs, the schemas have two fields and no nullable
fields, and this is hand-written Java, not generated bytecode. The pattern
does repeat across both runs: no clear gain for flat rows with dynamic
encoding, about 5% to 14% for nested rows with dynamic encoding, and about
11% to 44% with static encoding.
I also tried the cheap part without codegen: the existing loop, with the
static-encoding option resolved once at construction. That gave 8% to 9%
for static encoding on the encode side in one run and nothing clear
elsewhere. So most of the difference seems to come from the per-field call
sites. I did not profile to confirm that.
Limits I would expect:
- Wide schemas. By default HotSpot does not JIT-compile a method above
8,000 bytes of bytecode (DontCompileHugeMethods), and a method cannot
exceed 64 KB. By my estimate straight-line code needs more than 20 bytes
per field, so a 1,024-field schema must be split into chunks. I have not
measured that case.
I don't think this is a big deal. We could skip this extra optimization if
num_fields > N, falling back to the current generation.
- Generation cost. More bytecode per schema makes coder creation
slower than it is today, which is the opposite direction of this PR.
I suspect the difference will be somewhat minimal. Once we're already
generating classes, the cost of generating a few extra bytecodes is
unlikley to make a difference.
…
- Semantics. A baked-in static-encoding flag assumes that the row has
the same schema options as the coder. Today encode reads the option
from the schema of the row.
Not sure I understand this point?
So the two directions conflict. This PR drops the generated class to make
creation cheaper, and specialized codegen would spend more at creation to
make steady state faster. It would be a separate change, built on the
existing generator and not on this PR.
Straight-line coder used for the flat schema (BYTE field0, BYTES field1)
static final class FlatStraightCoder extends CustomCoder<Row> {
private static final ByteCoder FIELD0_CODER = ByteCoder.of();
private static final ByteArrayCoder FIELD1_CODER = ByteArrayCoder.of();
private final Schema schema;
private final boolean staticEncoding;
FlatStraightCoder(Schema schema) {
this.schema = schema;
this.staticEncoding =
schema.getOptions().getValueOrDefault("beam:option:row:static_encoding", false);
}
@OverRide
public void encode(Row value, OutputStream outputStream) throws IOException {
checkState(value.getFieldCount() == 2);
if (!staticEncoding) {
VAR_INT_CODER.encode(2, outputStream);
NULL_LIST_CODER.encode(EMPTY_BIT_SET, outputStream);
}
Byte field0 = value.getValue(0);
if (field0 != null) {
FIELD0_CODER.encode(field0, outputStream);
}
byte[] field1 = value.getValue(1);
if (field1 != null) {
FIELD1_CODER.encode(field1, outputStream);
}
}
@OverRide
public Row decode(InputStream inputStream) throws IOException {
Object[] fieldValues = new Object[2];
if (staticEncoding) {
fieldValues[0] = FIELD0_CODER.decode(inputStream);
fieldValues[1] = FIELD1_CODER.decode(inputStream);
} else {
// The field count and the null list still allow compatible schema changes.
int fieldCount = VAR_INT_CODER.decode(inputStream);
BitSet nullFields = NULL_LIST_CODER.decode(inputStream);
if (fieldCount > 0 && !nullFields.get(0)) {
fieldValues[0] = FIELD0_CODER.decode(inputStream);
}
if (fieldCount > 1 && !nullFields.get(1)) {
fieldValues[1] = FIELD1_CODER.decode(inputStream);
}
}
return Row.withSchema(schema).attachValues(fieldValues);
}
}
The nested variant has the same shape, with two FlatStraightCoder fields
in place of the byte coders. The benchmark setup checks that each variant
writes the same bytes as RowCoder.of(schema).
—
Reply to this email directly, view it on GitHub
<#39619?email_source=notifications&email_token=AFAYJVLMPUGS5YPP4CEEG6D5TAHP5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTMMBXGAZDANZXGAY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-6070207701>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFAYJVJU2IPCYCBJIIYG4VD5TAHP5AVCNFSNUABEKJSXA33TNF2G64TZHM2TAOJQGQZDINJ3JFZXG5LFHM2TANRVGEYTGMZXGKQXMAQ>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AFAYJVLTVKX2TDJWWRHVM335TAHP5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTMMBXGAZDANZXGAY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/AFAYJVK3QJWRKWUUTUPUBKL5TAHP5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTMMBXGAZDANZXGAY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39619 +/- ##
============================================
+ Coverage 56.16% 58.64% +2.47%
- Complexity 2288 13946 +11658
============================================
Files 1124 2593 +1469
Lines 178336 272140 +93804
Branches 1489 11286 +9797
============================================
+ Hits 100170 159585 +59415
- Misses 75645 106535 +30890
- Partials 2521 6020 +3499
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bvolpato
changed the title
|
Spending a lot of time up front in order to make flattened straight-line, unrolled coders, is almost certainly the right direction. Given today's tech:
If we can pass a few seconds of fuzz testing of arbitrary schemas, the chance that there's a weird one hiding is very low. We could also do exhaustive testing up to some depth (in other words move from 'quickcheck' model to 'smallcheck' model) and this would give a ton of assurance. |
|
We probably don't want to fully flatten for huge schemas, but we can easily
skip the unrolling if the schema has > N fields.
…On Tue, Oct 13, 2026 at 6:51 AM Kenn Knowles ***@***.***> wrote:
*kennknowles* left a comment (apache/beam#39619)
<#39619 (comment)>
Spending a lot of time up front in order to make flattened straight-line,
unrolled coders, is almost certainly the right direction.
Given today's tech:
- I think we could pretty reasonably get fully flattened and
concreteized generated coders (these were always possible, but hard to
implement and hard to get right, before)
- Test the generator against exact input/output matching of arbitrary
"slow version" reflective coders.
- Run the above test as a fuzz test of arbitrary generated schemas.
If we can pass a few seconds of fuzz testing of arbitrary schemas, the
chance that there's a weird one hiding is very low. We could also do
exhaustive testing up to some depth (in other words move from 'quickcheck'
model to 'smallcheck' model) and this would give a ton of assurance.
—
Reply to this email directly, view it on GitHub
<#39619?email_source=notifications&email_token=AFAYJVPPOWJDOUNT2YPSJST5TYXOPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTMMJUG4YTAMBRGAZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-6147100103>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFAYJVKARLMRVUQPOQFCFPT5TYXOPAVCNFSNUABEKJSXA33TNF2G64TZHM2TAOJQGQZDINJ3JFZXG5LFHM2TANRVGEYTGMZXGKQXMAQ>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AFAYJVMNOSLQX6R2IKZLTF35TYXOPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTMMJUG4YTAMBRGAZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/AFAYJVPPRAW4EGFECMOQUYL5TYXOPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTMMJUG4YTAMBRGAZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
4 participants
RowCoderGeneratorcreates a Byte Buddy class on each schema cache miss, although its encode and decode methods call shared loops. This change replaces per-schema generated classes with instances of oneCustomCoder<Row>implementation. Schema caching, component coders, encoding-position mapping, null handling, and the encode/decode logic remain in place. Encoding still reads the static-encoding option from the row's schema, while decoding reads it from the coder's schema.The recorded JMH runs measured fresh schema construction together with uncached coder generation. That combined operation took 48% to 97% less time; schema construction was also measured separately as a control. Nested dynamic decoding was 1.4% to 1.6% slower, and a reverse-order repeat measured a 4.6% slowdown for the small nested static case. Worker startup and pipeline runtime effects remain unmeasured, so the PR stays draft while the tradeoff is reviewed.
Java unit validation and Spotless passed before the coverage configuration update. The Codecov configuration excludes the added JMH measurement classes, following the existing benchmark exclusions.
Run the Java core tests with:
./gradlew -p sdks/java/core testBenchmark methodology, results, and commands
Large and nested schema benchmarks
The benchmarks cover 2 and 1,024 top-level fields, flat and nested rows, and both static and dynamic encoding. Each nested field contains a two-scalar-field row, so the largest nested case has 2,048 scalar leaves. Fields are non-null; nullable correctness remains covered by the unit tests. Benchmark setup verifies an encode/decode round trip.
JDK 21.0.11, JMH 1.34, one thread, two fresh JVM forks per case, and a fixed 256 MiB heap. Steady-state runs use three one-second warmup iterations and five one-second measurements per fork. Generation uses five warmups and ten single-shot measurements per fork, with a fresh schema UUID per invocation and a schema-construction control. The
generatemeasurements include schema construction; the separatebuildSchemacontrol is not subtracted. Both versions use the same benchmark and dependency classpath; the baseline substitutes the pre-changeRowCoderGeneratorfrom commit77a11347746d702040035bf673ae5596b8c0deba.In the full matrix, building a fresh schema and generating its uncached coder took 48% to 97% less time. Nested dynamic decoding took about 1.4% to 1.5% more time, while large dynamic encoding took about 1% less time. Those differences have non-overlapping JMH intervals in this run. The other steady-state intervals overlap, which does not prove equivalent performance.
Steady-state encoding and decoding
Units: ns/op; ± is JMH 99.9% confidence interval half-width.
Uncached generation and schema construction
Units: us/op; ± is JMH 99.9% confidence interval half-width.
Reverse-order nested decode confirmation
A second run, with the PR measured before the baseline, reproduces the dynamic nested decode cost at about 1.5% to 1.6%. It also measures a 4.6% cost for the small nested static case; the first pass measured 2.9% with overlapping intervals. The large nested static intervals still overlap.
Same protocol; PR ran before baseline. Intervals are 99.9%.
Reproduce the matrix on each implementation with:
addresses #123orfixes #456).CHANGES.mdwith noteworthy changes.See CONTRIBUTING.md for contribution guidelines.