reuvenlax commented on PR #39619: URL: https://github.com/apache/beam/pull/39619#issuecomment-6071773195
Interesting. Some comments inline: On Thu, Oct 8, 2026 at 6:21 PM Bruno Volpato ***@***.***> wrote: > *bvolpato* left a comment (apache/beam#39619) > <https://github.com/apache/beam/pull/39619#issuecomment-6070207701> > > 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 > <https://github.com/apache/beam/pull/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: > ***@***.***> > -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
