binary-signal commented on PR #3529:
URL: https://github.com/apache/fluss/pull/3529#issuecomment-4802236129
> Thanks @binary-signal great improvements! 🚀
>
> I have added couple suggestions.
>
> Another question: Do you have benchmarks / flamegraphs for
autoPartitionStrategy caching only? E.g, with partitioned table.
Good question and no, i don't have benchmarks / flamegraphs for
autoPartitionStrategy caching.
The benchmark was run on a non-partitioned log table. On that path the new
`isPartitioned()` guard short-circuits before `getAutoPartitionStrategy()` is
ever called, so the memoisation (patch 2) contributes ~nothing~ to those
results, the entire gain there comes from patch 1 skipping the per-record work.
The caching only matters for partitioned tables. There the guard passes, and
since Java evaluates the argument eagerly, `getAutoPartitionStrategy()` runs on
every record (`WriterClient.java:182`):
```java
private void doSend(WriteRecord record, WriteCallback callback) {
...
...
if (tableInfo.isPartitioned()) {
dynamicPartitionCreator.checkAndCreatePartitionAsync(
physicalTablePath,
tableInfo.getPartitionKeys(),
tableInfo.getTableConfig().getAutoPartitionStrategy()); //
arg evaluated per record
}
...
```
The cache turns that per-record call into a single volatile read
(`TableConfig.java:165`):
```java
public AutoPartitionStrategy getAutoPartitionStrategy() {
AutoPartitionStrategy s = autoPartitionStrategy; // volatile read on the
hot path
if (s == null) { // construct once
s = AutoPartitionStrategy.from(config);
autoPartitionStrategy = s;
}
return s;
}
```
Without it, each record paid a fresh `AutoPartitionStrategy.from(config)`,
which includes the `TimeZone.getTimeZone()` lookup that showed up in the
flamegraph (`AutoPartitionStrategy.java:57`):
```java
public static AutoPartitionStrategy from(Configuration conf) {
return new AutoPartitionStrategy(
conf.getBoolean(ConfigOptions.TABLE_AUTO_PARTITION_ENABLED),
conf.getString(ConfigOptions.TABLE_AUTO_PARTITION_KEY),
conf.get(ConfigOptions.TABLE_AUTO_PARTITION_TIME_UNIT),
conf.getInt(ConfigOptions.TABLE_AUTO_PARTITION_NUM_PRECREATE),
conf.getInt(ConfigOptions.TABLE_AUTO_PARTITION_NUM_RETENTION),
TimeZone.getTimeZone(conf.getString(ConfigOptions.TABLE_AUTO_PARTITION_TIMEZONE)));
// costly lookup
}
```
So I'd expect a similar flamegraph delta to the non-partitioned case
`getTimeZone()` + `AutoPartitionStrategy.from()` disappearing from the writer
hot path but I haven't measured it yet.
--
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]
