[GitHub] [flink-benchmarks] Myasuka commented on pull request #52: [FLINK-27214] Fix rescaling benchmark build failed

[GitBox <gi...@apache.org>]

Myasuka commented on PR #52:
URL: https://github.com/apache/flink-benchmarks/pull/52#issuecomment-1098018316

   Current problem is because `#rescaleRocksDB` executed multi times without call `#tearDownPerInvocation` after each call.
   
   From my understanding, if we take a look at the generated code by jmh, we can see the logic below:
   ~~~java
    public BenchmarkTaskResult rescaleRocksDB_AverageTime(InfraControl control, ThreadParams threadParams) throws Throwable {
           ...
               control.preSetup();
               l_rocksdbstatebackendrescalingbenchmarkexecutor0_0.setUpPerInvocation();
               control.announceWarmupReady();
               while (control.warmupShouldWait) {
                   l_rocksdbstatebackendrescalingbenchmarkexecutor0_0.rescaleRocksDB();
                   res.allOps++;
               }
          ...
   }
   ~~~
   If the `#rescaleRocksDB()` only executed once, that would be fine. However, we cannot ensure only once execution for this method within the while loop.
   If we change the annotataion to `Level.Invocation`, generated code would be:
   ~~~java
    public BenchmarkTaskResult rescaleRocksDB_AverageTime(InfraControl control, ThreadParams threadParams) throws Throwable {
           ...
               while (control.warmupShouldWait) {
                   l_rocksdbstatebackendrescalingbenchmarkexecutor0_0.setUpPerInvocation();
                   l_rocksdbstatebackendrescalingbenchmarkexecutor0_0.rescaleRocksDB();
                   l_rocksdbstatebackendrescalingbenchmarkexecutor0_0.tearDownPerInvocation();
                   res.allOps++;
               }
          ...
   }
   ~~~
   Even the while loop could be executed multi times, since we call the `setup` and `tearDown` around the benchamrk, there would be no problem.
   
   Since current execution time would be larger than one second, I think we would not hit the warning of inaccurate results.
   
   Thus, I am +1 for merging this fix.


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org