RFR: 7879: Automated Analysis taking very long time to produce results for Class Leak Rule and showing wrong results.

[Brice Dutheil]

On Sat, 20 Aug 2022 18:59:14 GMT, Suchita Chaturvedi <schaturvedi at openjdk.org> wrote:

> This PR takes care of optimizing and correcting the Class leak rule results on Automated Analysis Page.

I believe some changes are still required.

core/org.openjdk.jmc.flightrecorder.rules.jdk/src/main/java/org/openjdk/jmc/flightrecorder/rules/jdk/general/ClassLeakingRule.java line 105:

> 103: 
> 104: 	public static final TypedPreference<IQuantity> MAX_TIMEOUT = new TypedPreference<>(
> 105: 			"classLeaking.calculation.timeout", //$NON-NLS-1$

Just a suggestion

Suggestion:

			"classLeakingRuke.calculation.timeout", //$NON-NLS-1$

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

> 52: import org.openjdk.jmc.common.item.IType;
> 53: import org.openjdk.jmc.common.item.ItemFilters;
> 54: import org.openjdk.jmc.flightrecorder.rules.jdk.general.ClassLeakingRule;

I don't think that desirable to make `DefaultIItemResultSet` depend on `ClassLeakingRule`, this make every rule that use that class usage of that class having the same timeout as `ClassLeakingRule` (even if _as of today_ only 2 rule use this).

I believe the code will be better to add an override like this (adapt the timeout type if necessary), and of course the constructor of `DefaultIItemResultSet`.


  public class ItemResultSetFactory {
  	public IItemResultSet createResultSet(IItemCollection items, IItemQuery query)  {
  		return new DefaultIItemResultSet(items, query);
  	}
+ 	public IItemResultSet createResultSet(IItemCollection items, IItemQuery query, LinearQuantity timeout) {
+ 		return new DefaultIItemResultSet(items, query, timeout);
+ 	}
  }


And make `ItemResultSetFactory.createResultSet(IItemCollection items, IItemQuery query)` use the default timeout. Maybe the default can be `null` to indicate there's no timeout, I think `MINUTE.quantity(-1)` could work.

-----
Rule usages
https://github.com/openjdk/jmc/blob/1696e03a1fddb9cc68d32bf21a281ef24366e552/core/org.openjdk.jmc.flightrecorder.rules.jdk/src/main/java/org/openjdk/jmc/flightrecorder/rules/jdk/general/ClassLeakingRule.java#L220

https://github.com/openjdk/jmc/blob/1696e03a1fddb9cc68d32bf21a281ef24366e552/core/org.openjdk.jmc.flightrecorder.rules.jdk/src/main/java/org/openjdk/jmc/flightrecorder/rules/jdk/latency/BiasedLockingRevocationRule.java#L256

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

> 126: 								synchronized (data) {
> 127: 									data.add(row);
> 128: 								}

In my previous [comment](https://github.com/openjdk/jmc/pull/248/files#r633606757) on #248 I was thinking more about tweaking the collection, sorry for the lack of precision back then. So something like:


- private final ArrayList<Object[]> data = new ArrayList<>();
+ private final List<Object[]> data = Collections.synchronizedList(new ArrayList<Object>());


Or if the insertion order is irrelevant one might think of a `ConcurrentLinkedQueue` instead during the calculation, and copy the elements in the array list once done.

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

> 132: 				} finally {
> 133: 					exec.shutdown();
> 134: 					exec.awaitTermination(ClassLeakingRule.CONFIGURED_TIMEOUT, TimeUnit.MINUTES);

I believe the proper executor shutdown sequence would be something like this

Suggestion:

				} finally {
				    exec.shutdown();
					try {
						if (!exec.awaitTermination(60, TimeUnit.SECONDS)) {
							exec.shutdownNow();
							
							// possibly wait for other task to respond to cancellation, possibly not needed in this case. But useful to keep, if this code gets extracted away.
							if (!pool.awaitTermination(timeout.clampedLongValueIn(MINUTE), TimeUnit.MINUTES)) {
								// TODO notify error that pool did not terminate
							}
						}
					} catch (InterruptedException ie) {
						exec.shutdownNow();
						Thread.currentThread().interrupt();
					}

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

Changes requested by bdutheil (Author).

PR: https://git.openjdk.org/jmc/pull/419