RFR: 7248: JMC7/8 Automated Analysis taking very long time to produce results for Class Leak Rule. [v2]
Henrik Dafgård
hdafgard at openjdk.java.net
Wed Nov 24 19:06:10 UTC 2021
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
More information about the jmc-dev
mailing list