[review needed] speed up RBClassNotReferencedRule

Hi,

Review needed for

        https://pharo.fogbugz.com/f/cases/16074/Is-RBClassNotReferencedRule-200x-slower-in-Pharo-4-vs-Pharo-3

Speedup can be checked with:

rule:=RBClassNotReferencedRule new.
environment:=RBBrowserEnvironment new forPackageNames: #('Fuel').
[RBSmalllintChecker runRule: rule onEnvironment:
environment] timeToRun


Pharo5: "0:00:00:13.526"
Pharo5 with fix: "0:00:00:00.761"
Pharo3:  0:00:00:00.547

Checking all classes for usage:

[Smalltalk allClassesAndTraits select: [ : each | each isUsed not]] timeToRun

still takes 1:40 to find 600 unused classes.

This shows that adding a cache for “referenced classes” per environment would speed this
up further. But it should be general, not just for RB or even RB Code Critique.

The nice things is that code environments only change when you add classes or methods,
so adding a very general cache should be quite possible.

Another thing we should add is a way for classes to define “I am used”. E.g this
now finds all Configurations, all the Manifests and all cases like the RB Rules themselves.
All these should know that they are not useless even though there are no references.

        Marcus

Marcus Denker-4 wrote

Hi,

Review needed for

        https://pharo.fogbugz.com/f/cases/16074/Is-RBClassNotReferencedRule-200x-slower-in-Pharo-4-vs-Pharo-3

Speedup can be checked with:

rule:=RBClassNotReferencedRule new.
environment:=RBBrowserEnvironment new forPackageNames: #('Fuel').
[RBSmalllintChecker runRule: rule onEnvironment:
environment] timeToRun


Pharo5: "0:00:00:13.526"
Pharo5 with fix: "0:00:00:00.761"
Pharo3:  0:00:00:00.547

Checking all classes for usage:

[Smalltalk allClassesAndTraits select: [ : each | each isUsed not]] timeToRun

still takes 1:40 to find 600 unused classes.

This shows that adding a cache for “referenced classes” per environment would speed this
up further. But it should be general, not just for RB or even RB Code Critique.

The nice things is that code environments only change when you add classes or methods,
so adding a very general cache should be quite possible.

Another thing we should add is a way for classes to define “I am used”. E.g this
now finds all Configurations, all the Manifests and all cases like the RB Rules themselves.
All these should know that they are not useless even though there are no references.

        Marcus

Thanks for working on this.   I have a use for it this week.  

For methods RB allows one to add a pragma e.g.:

        <lint: 'Methods implemented but not sent' rationale: 'this code plays a role in examining the system' author: 'pad'>

and described here: http://www.lukas-renggli.ch/blog/ignoring-lint-rules.  Maybe we can hijack that mechanism with a class side method e.g.

rbNotReferenced
         <lint>


Do your modifications test sends of subclassesDo: and friends?

OR

#(#MyClass1 #MyClass2 #MyClass3) collect:[:ea | (Smalltalk at: ea) doSomething].

The reason I ask is I'm planning to do a bunch of cleanup of a package and it would be nice to not worry about those cases getting missed

What I did is to implement #isUsed like this in e.g the hierarchy of RBLintRule:

isUsed
        "all my sublasses are used"
        ^self name ~= ‘RBLintRule'

this way if RBLintRule is not used, it is seen by the Critique, but all subclasses
are used by default.

I implemented that in TestCase, too, and removed the explicit check for TestCase
subclasses from the rule.

        Marcus

This is a nice thing. In the same way we can deal with abstract classes. So if someone develops an abstract class with an intent to use it, he can specify that it’s ok to use the class.

Also is there any work on pragmas for classes, at some point I’ve heard something about it but can’t recall.

I will try to hack a bit this weekend and see if I can add some kind of automation to marking a class as used. I.e. if you see a critic, you should be able to mark the class as being used with 1-2 clicks. If you are developing some king of facade, you should know that you will not reference it from your library.

Uko

>
> Marcus

True.
> Also is there any work on pragmas for classes, at some point I’ve heard something about it but can’t recall.
>
Yes, the question is where to put it syntactically and how to save it in monticello… could be interesting to have.

> I will try to hack a bit this weekend and see if I can add some kind of automation to marking a class as used. I.e. if you see a critic, you should be able to mark the class as being used with 1-2 clicks. If you are developing some king of facade, you should know that you will not reference it from your library.
>

ok.

        Marcus

On Fri, 2015-09-18 at 14:48 +0200, Marcus Denker wrote:
After a class declaration like this:

LLVMDisposableObject subclass:#LLVMDIBuilder
        instanceVariableNames:''
        classVariableNames:''
        poolDictionaries:'LLVMModuleFlagBehavior'
        category:'LLVM-S-Core'    

Annotation key: 'disposable:' value: 'true'

This ANSI-compatible way (see page 39 of NCITS J20 DRAFT
of ANSI Smalltalk Standard, rev 1.9).

Perhaps nice way is

Annotation disposable: true


>  and how to save it in monticello…

In the source as above. In the binary, in a separate
entry in the .zip so the other MC implementation won't
crash but silently ignore them.

Yes, that looks good. I made a note (no idea when I will come around implementing it, though).

        Marcus

Sorry guys but this is terrible because it is implicitly scoped.
 From an analysis point of view we have to rely on order to A better way would be

LLVMDIBuilder disposable: true
or
LLVMDIBuilder annotationKey: #disposable: value: true