RFR: 7248: JMC7/8 Automated Analysis taking very long time to produce results for Class Leak Rule.

[Brice Dutheil]

On Tue, 11 May 2021 22:35:08 GMT, Suchita Chaturvedi <schaturvedi at openjdk.org> wrote:

> This PR addresses the optimization of Class Leak Rule.
> 
> Please refer the JBS for the sample JFR which was taking ~90 minutes to generate results for Class Leak Rule. After the optimization time is reduced to ~10-15 minutes.
> 
> Please review the change and let me the know if there are any comments / suggestions.

Hi,

I believe the executor code has some issues that should be tackled.

core/org.openjdk.jmc.flightrecorder.rules.jdk/src/main/java/org/openjdk/jmc/flightrecorder/rules/jdk/util/DefaultIItemResultSet.java line 76:

> 74: 		} catch (Exception e) {
> 75: 			e.printStackTrace();
> 76: 		}

Don't swallow the interrupted exception, if this happens, this mean the code calling awaitTermination has been interrupted, the code should make sure the current thread is interrupted.

core/org.openjdk.jmc.flightrecorder.rules.jdk/src/main/java/org/openjdk/jmc/flightrecorder/rules/jdk/util/DefaultIItemResultSet.java line 108:

> 106: 			if (aggregate != null) {
> 107: 				int SUM_NUM_THREADS = Runtime.getRuntime().availableProcessors();
> 108: 				ExecutorService exec = Executors.newFixedThreadPool(SUM_NUM_THREADS);

I wonder if a `ForkJoinPool` might be better suited here (maybe with `Executors.newWorkStealingPool`), in which case the code won't need to look for the number of avaialble processors.
Going furhter it might be even useful to consider `Stream::parallelStream`?

core/org.openjdk.jmc.flightrecorder.rules.jdk/src/main/java/org/openjdk/jmc/flightrecorder/rules/jdk/util/DefaultIItemResultSet.java line 108:

> 106: 			if (aggregate != null) {
> 107: 				int SUM_NUM_THREADS = Runtime.getRuntime().availableProcessors();
> 108: 				ExecutorService exec = Executors.newFixedThreadPool(SUM_NUM_THREADS);

This might be useful to have a named threads. Yet I never tired this with FJP.
Maybe mission control do have some facility in that regard ?

core/org.openjdk.jmc.flightrecorder.rules.jdk/src/main/java/org/openjdk/jmc/flightrecorder/rules/jdk/util/DefaultIItemResultSet.java line 126:

> 124: 									row[column + j] = rowCollection.getAggregate(aggregators.get(j));
> 125: 								}
> 126: 								data.add(row);

Shouldn't this access be synchronized ?

core/org.openjdk.jmc.flightrecorder.rules.jdk/src/main/java/org/openjdk/jmc/flightrecorder/rules/jdk/util/DefaultIItemResultSet.java line 134:

> 132: 				}
> 133: 				// Higher timeout value added for worst case
> 134: 				exec.awaitTermination(1, TimeUnit.HOURS);

It may be worth to make this value configurable. IE let hte use know that how long it will allow for the background job to run.

Also, it may be worthy to actually enforce the shutdown once this delay is over. IE soemthing like that


try { 
  // allow for budget (default 1h) to process the aggregates
  if(!exec.awaitTermination(delay, delayUnit)) {
    exec.shutdownNow();
    // last grace period
    if (!pool.awaitTermination(1, TimeUnit.MINUTES)) {
      // report an issue with the pool
    }
  }
} catch (InterruptedException e) {
  Thread.currentThread().interrupt();
}


Also I think this code could moved in the same scope as the shutdown call.

-------------

Changes requested by bric3 at github.com (no known OpenJDK username).

PR: https://git.openjdk.java.net/jmc/pull/248