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

[Henrik Dafgård]

On Tue, 27 Jul 2021 14:37:52 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.
>
> Suchita Chaturvedi has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Calculation maximum timeout is set via preference and hardcoding removed.

I also agree with Brice's suggestions for changes.

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

> 157: 		IItemCollection items, IPreferenceValueProvider valueProvider, IResultValueProvider dependencyResults) {
> 158: 		int warningLimit = (int) valueProvider.getPreferenceValue(WARNING_LIMIT).longValue();
> 159: 		CONFIGURED_TIMEOUT = (int) valueProvider.getPreferenceValue(MAX_TIMEOUT).longValue();

This adds state to a rule and is going to cause issues if you're evaluating rules in parallel with different timeout configurations. Any such field must be final instead.

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

> 81: 	private void calculateData(IItemCollection input) throws InterruptedException {
> 82: 		input = input.apply(query.getFilter());
> 83: 		final IItemCollection newInput = input;

Can't this be done in the calling function instead of here? This should also be where the timeout is passed into this method instead of as a public static variable.

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

> 133: 				}
> 134: 				// Timeout can be configured in prefrences (default value is set to 5 minutes)
> 135: 				exec.awaitTermination(ClassLeakingRule.CONFIGURED_TIMEOUT, TimeUnit.MINUTES);

This means that the generic DefaultIItemResultSet class has an explicit dependency on the configuration of one rule, much better to add a timeout parameter at the creation of the instance.

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

Changes requested by hdafgard (Reviewer).

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