Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate benchmarks to druid-benchmarks module #17693

Open
Akshat-Jain opened this issue Feb 3, 2025 · 0 comments
Open

Migrate benchmarks to druid-benchmarks module #17693

Akshat-Jain opened this issue Feb 3, 2025 · 0 comments

Comments

@Akshat-Jain
Copy link
Contributor

Description

Currently, we have some benchmarks which are present outside of druid-benchmarks module:

  • TopNQueryRunnerBenchmark.java
  • ApproximateHistogramErrorBenchmark.java
  • CardinalityAggregatorBenchmark.java
  • JavaScriptAggregatorBenchmark.java
  • TopNBinaryFnBenchmark.java
  • MemcachedCacheBenchmark.java
  • HyperLogLogCollectorBenchmark.java
  • BitmapCreationBenchmark.java
  • CostBalancerStrategyBenchmark.java

These should be moved to the druid-benchmarks module.

I spent some time in attempting to move them, the diff for a couple of which can be seen here. But it's not trivial to move them, so I'm sharing my findings and the diff as a reference point, in case anyone would like to take it forward.

  • TopNQueryRunnerBenchmark.java: This has been moved to the druid-benchmarks module in the diff, but it doesn't work because of StupidPool leaks. But the same problem exists in the existing TopNQueryRunnerBenchmark.java.
  • ApproximateHistogramErrorBenchmark.java: This has been moved to the druid-benchmarks module in the diff. But the existing ApproximateHistogramErrorBenchmark.java was printing/reporting the number of errors, which doesn't seem possible to do with JMH.
    • To add a custom column in JMH benchmark results, I tried using AuxCounters, but JMH doesn't output the value in AuxCounters class without aggregating them over the run of the benchmark (across all iterations). So this doesn't work.
    • AuxCounters doesn't support SingleShot mode, which is what has been done in the diff for now. Instead of adding it in the benchmark results, we are using SingleShot mode, and printing the number of errors, since JMH can't report it as an output metric. So the benchmark results themselves don't really add much of a value IMO.
  • The following benchmarks error out with java.lang.NoSuchMethodError: 'void com.google.common.io.Closeables.closeQuietly(java.io.Closeable)':
    • CardinalityAggregatorBenchmark.java
    • JavaScriptAggregatorBenchmark.java
    • TopNBinaryFnBenchmark.java
    • MemcachedCacheBenchmark.java
    • HyperLogLogCollectorBenchmark.java
  • BitmapCreationBenchmark.java: It is ignored with @Ignore.
  • CostBalancerStrategyBenchmark.java: We have 2 files of the same name
    • First one is ignored with @Ignore.
    • Second one is already in druid-benchmarks module. Not sure if we can remove the first one directly, it is marked with @Ignore anyway.

For each of the above, we should ideally try to analyze the value they add (since most of them are non-functional currently - they either error out or are marked with @Ignore). If they add value, then they should be moved to the druid-benchmarks module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant