Change the way we locate and filter Rules

[Mauin]

Currently the Detektor will do the following during a run:

• Get a list of KtFile
• For each file it will get all RuleSetProviders
• Pass file to RuleSetProvider which will visit all rules (or do test pattern filtering)
• Rule does the active (and suppression) checks

However between one file and the next, the rules we want to run and the config will not change. We want to run each file over the same set of Rules. The test pattern is a slight exception here but the same still applies for each file that matches the test pattern.

My suggestion is: Instead of getting every single rule for every single file and doing active checks in the rules we could precompute the list of Rules we're interested in when we create the Detektor (and another list for the rules to run on the files matching the test pattern). Then we have all rules we care about and simply pass each file to each of the identified (and relevant) rules.

I see two benefits in doing this:

• Less overhead as we compute and filter the list of rules once at the beginning of the run instead of once per KtFile
• Central point for filtering based on config values. Currently we do some filtering in Detektor some filtering in RuleSetProvider (test pattern) and some filtering in Rule. If we would precompute the list of Rules we could handle the test pattern and active filtering in that component instead of spreading this out over multiple components.

I played around with this a bit and this would essentially make RuleSetProvider just return it's list of Rules and the Detektor would then operate on the Rules directly instead of passing files to the RuleSetProviders.

I have a branch in which I tried this in a bit of a hacky way. If we agree that this might be worth pursuing I would continue working on it. (see: https://github.com/arturbosch/detekt/compare/master...Mauin:mr/rule_set_perf?expand=1 Detektor and RuleManager are the interesting classes)

[arturbosch]

More information on possible api changes:

In some other issue we discussed the overhead of creating all rules upfront and then filter the active ones.
This is due to the RuleSetProvider design which just returns a rule set with all rules created. Rule writers can pass arguments to the constructors as they like. Also the config.
One idea would be to change this to a list of KClass and create them via reflection after filtering.
But this would make configuring the rules difficult and everything must be lazy.

I see your point in creating the rules one time and reuse them. That was also the actual design.
Reusing the rules is hard as all rules must make sure not to be mutable. The easiest way to remove this mutability is to not reuse them.

Have you benchmark'd the creation and filtering or was this just an idea?
I don't know if the creation of configured rules is such on overhead. In my opinion the biggest part is to create the KotlinCoreEnvironment and then traversing the psi times the amount of rules.

[schalkms]

I agree with @arturbosch that a benchmark would make sense here.
If we conclude that @Mauin approach is faster, this should be changed.

Either way, I think this is a topic which should be addressed after v1.
What do you think?

[Mauin]

I ran some benchmarks with and without the changes and there is no noticable performance impact. I also see the point of reusing rules and some of the problems that come with it. I'll close this issue.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related topics.