googleapis / googleapis/google-cloud-java
[java-bigtable] MutateRowsBatchingDescriptor.createResource() builds the entry proto twice for every batched element
- Lenguaje dominante
- Java
- Estrellas
- 2.1k
- Forks
- 1.2k
- Merge medio
- 1 d 23 h
- PR fusionados (30 d)
- 154
Descripción
Version: `google-cloud-bigtable` 2.80.0. `MutateRowsBatchingDescriptor.java` is byte-identical at
`main` today, so the line numbers below apply there too.
### Summary
`MutateRowsBatchingDescriptor.createResource()` builds the same `MutateRowsRequest.Entry` twice for
every element added to a bulk-mutation batcher: once to read its serialized size, once to read its
mutation count. `RowMutationEntry.toProto()` constructs a fresh proto on each call, so the second
construction is pure overhead on the batching hot path.
### Detail
```java
// MutateRowsBatchingDescriptor.java:98-106
@Override
public long countBytes(RowMutationEntry entry) {
return entry.toProto().getSerializedSize();
}
@Override
public BatchResource createResource(RowMutationEntry element) {
long byteCount = countBytes(element);
return MutateRowsBatchResource.create(1, byteCount, element.toProto().getMutationsCount());
}
```
`countBytes(element)` constructs one proto; `element.toProto().getMutationsCount()` constructs
another. Nothing is shared between them, because `toProto()` memoizes nothing:
```java
// RowMutationEntry.java:203-214
@InternalApi
public MutateRowsRequest.Entry toProto() {
Preconditions.checkArgument(...);
return MutateRowsRequest.Entry.newBuilder()
.setRowKey(key)
.addAllMutations(mutation.getMutations())
.build();
}
```
This runs per element, not per batch — `BatcherImpl.add()` calls `createResource` for every element
it accepts (`BatcherImpl.java:232`, gax 2.82.0), before it reserves flow-control resources.
### Cost
Measured on OpenJDK 21.0.5 (aarch64, `-Xmx4g`, default collector) against `google-cloud-bigtable`
2.80.0 and `gax` 2.82.0, with entries pre-built into a pool. The two arms are the proto expressions
of `createResource` as it ships and with the proto built once; allocation is sampled over the same
loop as the timing.
**Allocated bytes per element, which is the reliable half — exactly halved, in every fork and every
shape:**
| Entry shape | serialized size | today | one construction | change |
|---|---|---|---|---|
| 1 cell, 64 B value | 88 B | 352 B | 176 B | -50.0% |
| 8 cells, 128 B values | 1191 B | 672 B | 336 B | -50.0% |
| 64 cells, 128 B values | 9533 B | 2464 B | 1232 B | -50.0% |
| 1000 cells, 128 B values | 149 897 B | 32 416 B | 16 208 B | -50.0% |
**I am deliberately not quoting a wall-clock percentage.** The end-to-end A/B on my machine ranged
from -73% to +23% for the same shape across JVM forks, which is noise rather than signal, and I
would rather report that than a flattering number. What is measurable in isolation is the cost of
one `toProto()` construction, which is what the second call adds — a separate arm, three forks,
4 s warmup and 7 timed iterations of >=2 s each:
| Entry shape | one construction |
|---|---|
| 1 cell, 64 B value | 27.1 ns |
| 8 cells, 128 B values | 95.1 ns |
| 64 cells, 128 B values | 213.7 ns |
| 1000 cells, 128 B values | 2433.1 ns |
The allocation is reference copying rather than payload copying — `ImmutableList.build()`, an
`Entry.Builder`, `addAllMutations` and `build()` — which is why it grows at roughly 16 B per
mutation (two eight-byte references) rather than with the entry's byte size.
Note the *serialized size walk* is not what is duplicated: `Mutation.addMutation()` already calls
`getSerializedSize()` on each cell as it is added, and protobuf memoizes size per message instance,
so the child protos shared into every `Entry` are already warm. It is the construction alone that
is paid twice.
### Suggested fix
Build the proto once and read both values from it:
```java
@Override
public BatchResource createResource(RowMutationEntry element) {
MutateRowsRequest.Entry proto = element.toProto();
return MutateRowsBatchResource.create(1, proto.getSerializedSize(), proto.getMutationsCount());
}
```
Behaviour-preserving: the two values are read from an identical proto. `countBytes` stays as it is —
it remains part of the `BatchingDescriptor` contract and is still called from
`TracedBatchingCallable` and the interface's own default `createResource`.
I have this change ready with tests and will open a PR referencing this issue.
### Impact
Every application using `BigtableDataClient.newBulkMutationBatcher(...)`, on every mutation. It is
garbage-collection pressure and a constant CPU factor rather than a correctness problem, and it
grows with the number of mutations per row, so the heaviest writers pay the most.
Guía de contribución
Evaluación
Este issue todavía no se ha evaluado.