googleapis / googleapis/google-cloud-java
[java-bigtable] MutateRowsBatchingDescriptor.createResource() builds the entry proto twice for every batched element
- Langage dominant
- Java
- Étoiles
- 2.1k
- Forks
- 1.2k
- Merge moyen
- 1 j 23 h
- PR mergées (30 j)
- 154
Description
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.
Guide de contribution
Ouvrir le guide de contribution
Évaluation
Cette issue n'a pas encore été évaluée.