Performance review for an inspection.

Hi all,

Before I refactor this inspection, I'd like to know if it can be
optimized for speed.
The code is very simple, and is heavily based on the old
ComparingReferencesInspection example, from Max.


What it does: it looks for assignement to fields that are annotated with
@AlmostFinal:

public class Foo {
@AlmostFinal public int size ;
}

Assignements like
Foo foo = new Foo();
foo.size = 1;

are forbidden, except in the class Foo itself.


Thanks in advance.

Alain





1 package com.ravet.annotations;
2
3 import com.intellij.codeHighlighting.HighlightDisplayLevel;
4 import com.intellij.codeInspection.InspectionManager;
5 import com.intellij.codeInspection.LocalInspectionTool;
6 import com.intellij.codeInspection.ProblemDescriptor;
7 import com.intellij.codeInspection.ProblemHighlightType;
8 import com.intellij.psi.*;
9 import com.intellij.psi.util.PsiTreeUtil;
10 import org.jetbrains.annotations.Nullable;
11
12 import java.util.ArrayList;
13 import java.util.Arrays;
14 import java.util.List;
15
16 public class AlmostFinalInspection
17 extends LocalInspectionTool
18 {
19 private static final String TRACKED_ANNOTATION =
AlmostFinal.class.getName ();
20
21 public String getDisplayName () { return "@AlmostFinal"; }
22 public String getGroupDisplayName () { return
"Annotations-based"; }
23 public String getShortName () { return "AlmostFinal"; }
24
25 public HighlightDisplayLevel getDefaultLevel () { return
HighlightDisplayLevel.ERROR; }
26 public boolean isEnabledByDefault () { return
true; }
27
28
29
30 @Nullable public ProblemDescriptor[] checkClass (PsiClass
aClass, InspectionManager manager, boolean isOnTheFly)
31 {
32 List problemList = null; 33 PsiClassInitializer[] initializers = aClass.getInitializers (); 34 for (PsiClassInitializer initializer : initializers) { 35 ProblemDescriptor[] problemDescriptors = analyzeCode (initializer, manager); 36 if (problemDescriptors != null) { 37 if (problemList == null) { 38 problemList = new ArrayList(); 39 } 40 problemList.addAll (Arrays.asList (problemDescriptors)); 41 } 42 } 43 44 return problemList == null 45 ? null 46 : (ProblemDescriptor[]) problemList.toArray (new ProblemDescriptor[problemList.size ()]); 47 } 48 49 public ProblemDescriptor[] checkMethod (PsiMethod method, InspectionManager manager, boolean isOnTheFly) 50 { 51 return analyzeCode (method.getBody (), manager); 52 } 53 54 @Nullable 55 private static ProblemDescriptor[] analyzeCode (PsiElement where, final InspectionManager manager) 56 { 57 if (where == null) 58 return null; 59 60 final List[] problemList = new ArrayList[]{null}; 61 where.accept (new PsiRecursiveElementVisitor() 62 { 63 public void visitAssignmentExpression (PsiAssignmentExpression expression) 64 { 65 super.visitAssignmentExpression (expression); 66 67 PsiExpression lhs = expression.getLExpression(); 68 if (!(lhs instanceof PsiReferenceExpression)) { 69 return ; 70 } 71 72 PsiElement referent = ((PsiReference) lhs).resolve (); 73 if (referent == null) { 74 return; 75 } 76 77 if (!(referent instanceof PsiField)) { 78 return; 79 } 80 PsiField theField = (PsiField) referent; 81 82 boolean theFieldIsAlmostFinal = theField.getModifierList () 83 .findAnnotation (TRACKED_ANNOTATION) != null; 84 if (theFieldIsAlmostFinal) { 85 PsiClass expressionParentClass = PsiTreeUtil.getParentOfType (expression, PsiClass.class); 86 if (theField.getParent ().equals ( expressionParentClass)) 87 return ; 88 if (problemList[0] == null) { 89 problemList[0] = new ArrayList]]>();
90 }
91 problemList[0].add
(manager.createProblemDescriptor (lhs,
92 "Cannot assign a value to an
@AlmostFinal field.",
93 null,
94
ProblemHighlightType.GENERIC_ERROR_OR_WARNING));
95 }
96 }
97 });
98
99 return problemList[0] == null
100 ? null
101 : (ProblemDescriptor[]) problemList[0].toArray (new
ProblemDescriptor[problemList[0].size ()]);
102 }
103
104
105 }

17 comments
Comment actions Permalink


Well, performance-wise, it's gonna be slowish, but not horrible. You're doing a resolve() on every assignment to a variable, and that's gonna add up. It won't be atrocious, but it would be in the bottom fifty or so IG inspections for performance. Don't see any other way to accomplish what you're trying to.

Also, you want to use field.getContainingClass(), rather than field.getParent(), and you want to check it for null before using it.

All of that said, I don't honestly understand the use-case for this. When I originally saw "almost final" I figured it would be "like final, except that you can assign it in
clone() and readObject()". That would be handy, to get around the constraints of those methods. Not sure what you're trying do do with your semantics.

--Dave Griffith

0
Comment actions Permalink

If I were you, I'd rather write
public void visitReferenceExpression(PsiReferenceExpression
expression) {
PsiElement element = expression.resolve();
if (element instanceof PsiField) {
PsiField field = ((PsiField)element);
boolean isAnnotated = field.getModifierList().findAnnotation
(TRACKED_ANNOTATION);
if (isAnnotated && PsiUtil.isAccessedForWriting(expression) &&
accesedOutsideClass()) {
registerProblem();
return;
}
}
super.visitReferenceExpression(expression);
}

This way it would also catch something like x.field++ or --x.field.

I'd also renamed @AlmostFinal to @ReadOnly.

--
regards,
--
Alexey Kudravtsev
Software Developer
JetBrains, Inc, http://www.jetbrains.com
"Develop with pleasure!"


"Alain Ravet" <alain.ravet@biz.tiscali.be> wrote in message
news:d6sj5m$l5c$1@is.intellij.net...

Hi all,

>

Before I refactor this inspection, I'd like to know if it can be optimized
for speed.
The code is very simple, and is heavily based on the old
ComparingReferencesInspection example, from Max.

>
>

What it does: it looks for assignement to fields that are annotated with
@AlmostFinal:

>

public class Foo {
@AlmostFinal public int size ;
}
Assignements like
Foo foo = new Foo();
foo.size = 1;
are forbidden, except in the class Foo itself.

>
>

Thanks in advance.

>

Alain

>
>
>
>
>

1 package com.ravet.annotations;
2
3 import com.intellij.codeHighlighting.HighlightDisplayLevel;
4 import com.intellij.codeInspection.InspectionManager;
5 import com.intellij.codeInspection.LocalInspectionTool;
6 import com.intellij.codeInspection.ProblemDescriptor;
7 import com.intellij.codeInspection.ProblemHighlightType;
8 import com.intellij.psi.*;
9 import com.intellij.psi.util.PsiTreeUtil;
10 import org.jetbrains.annotations.Nullable;
11
12 import java.util.ArrayList;
13 import java.util.Arrays;
14 import java.util.List;
15
16 public class AlmostFinalInspection
17 extends LocalInspectionTool
18 {
19 private static final String TRACKED_ANNOTATION =
AlmostFinal.class.getName ();
20
21 public String getDisplayName () { return "@AlmostFinal"; }
22 public String getGroupDisplayName () { return
"Annotations-based"; }
23 public String getShortName () { return "AlmostFinal"; }
24
25 public HighlightDisplayLevel getDefaultLevel () { return
HighlightDisplayLevel.ERROR; }
26 public boolean isEnabledByDefault () { return
true; }
27
28
29
30 @Nullable public ProblemDescriptor[] checkClass (PsiClass aClass,
InspectionManager manager, boolean isOnTheFly)
31 {
32 List<ProblemDescriptor> problemList = null;
33 PsiClassInitializer[] initializers = aClass.getInitializers ();
34 for (PsiClassInitializer initializer : initializers) {
35 ProblemDescriptor[] problemDescriptors = analyzeCode
(initializer, manager);
36 if (problemDescriptors != null) {
37 if (problemList == null) {
38 problemList = new ArrayList<ProblemDescriptor>();
39 }
40 problemList.addAll (Arrays.asList
(problemDescriptors));
41 }
42 }
43
44 return problemList == null
45 ? null
46 : (ProblemDescriptor[]) problemList.toArray (new
ProblemDescriptor[problemList.size ()]);
47 }
48
49 public ProblemDescriptor[] checkMethod (PsiMethod method,
InspectionManager manager, boolean isOnTheFly)
50 {
51 return analyzeCode (method.getBody (), manager);
52 }
53
54 @Nullable
55 private static ProblemDescriptor[] analyzeCode (PsiElement where,
final InspectionManager manager)
56 {
57 if (where == null)
58 return null;
59
60 final List[] problemList = new ArrayList[]{null};
61 where.accept (new PsiRecursiveElementVisitor()
62 {
63 public void visitAssignmentExpression
(PsiAssignmentExpression expression)
64 {
65 super.visitAssignmentExpression (expression);
66
67 PsiExpression lhs =
expression.getLExpression();
68 if (!(lhs instanceof PsiReferenceExpression)) {
69 return ;
70 }
71
72 PsiElement referent = ((PsiReference)
lhs).resolve ();
73 if (referent == null) {
74 return;
75 }
76
77 if (!(referent instanceof PsiField)) {
78 return;
79 }
80 PsiField theField = (PsiField) referent;
81
82 boolean theFieldIsAlmostFinal =
theField.getModifierList ()
83 .findAnnotation (TRACKED_ANNOTATION) !=
null;
84 if (theFieldIsAlmostFinal) {
85 PsiClass expressionParentClass =
PsiTreeUtil.getParentOfType (expression, PsiClass.class);
86 if (theField.getParent ().equals (
expressionParentClass))
87 return ;
88 if (problemList[0] == null) {
89 problemList[0] = new
ArrayList<ProblemDescriptor>();
90 }
91 problemList[0].add
(manager.createProblemDescriptor (lhs,
92 "Cannot assign a value to an
@AlmostFinal field.",
93 null,
94
ProblemHighlightType.GENERIC_ERROR_OR_WARNING));
95 }
96 }
97 });
98
99 return problemList[0] == null
100 ? null
101 : (ProblemDescriptor[]) problemList[0].toArray (new
ProblemDescriptor[problemList[0].size ()]);
102 }
103
104
105 }



0
Comment actions Permalink

Eeep! Performance-wise that would be horrible. The number-one way to improve performance in inspections is to keep the number of resolve() calls to an absolute minimum. Check absolutely every possible gaurd condition before you do any resolution. Your solution would easily be slower than every inspection currently in IG.

You're right that he needs to check for ++ or --, though.

--Dave Griffith

0
Comment actions Permalink

You should probably ask for PsiUtil.isAccessedForWriting(expression) before
expression.resolve() though as resolve() seems to be an expensive operation.

+1 for @ReadOnly

Sascha

Alexey Kudravtsev (JetBrains) wrote:

If I were you, I'd rather write
public void visitReferenceExpression(PsiReferenceExpression
expression) {
PsiElement element = expression.resolve();
if (element instanceof PsiField) {
PsiField field = ((PsiField)element);
boolean isAnnotated = field.getModifierList().findAnnotation
(TRACKED_ANNOTATION);
if (isAnnotated && PsiUtil.isAccessedForWriting(expression) &&
accesedOutsideClass()) {
registerProblem();
return;
}
}
super.visitReferenceExpression(expression);
}

This way it would also catch something like x.field++ or --x.field.

I'd also renamed @AlmostFinal to @ReadOnly.

0
Comment actions Permalink

Dave


> All of that said, I don't honestly understand the use-case for this.


When I can, and when it makes sense, I use public final fields, rather
than getters. It's cleaner, shorter, and it tells me that the class is
immutable.
While modifying code, I sometimes have to remove the 'final' for a given
field to allow it to be set in some other way. As it's - probably -
temporary, and I hate mixing getters and direct field access, and I hate
adding getters for all the fields, I just remove the final, for this one
field. Temporarily.

This annotation is for cases like this. It suits my coding style, but I
would understand if the rest of the world were to find it useless.

Alain



0
Comment actions Permalink

Alexey

>I'd also rename @AlmostFinal to @ReadOnly.

>

It's much better. Actually, I even find it too good for my modest
inspection support.
I feel like stealing a universally acclaimed and understood word for a
very annecdotical and limited purpose.

What do you think?

Alain

0
Comment actions Permalink

Dave,
while resolving is really an expensive operation, the results of calling
resolve are cached, so it's not that horribly expensive:)
Eugene.

"Dave Griffith" <dave.griffith@cnn.com> wrote in message
news:4071147.1116854928131.JavaMail.javamailuser@localhost...

Eeep! Performance-wise that would be horrible. The number-one way to

improve performance in inspections is to keep the number of resolve() calls
to an absolute minimum. Check absolutely every possible gaurd condition
before you do any resolution. Your solution would easily be slower than
every inspection currently in IG.
>

You're right that he needs to check for ++ or --, though.

>

--Dave Griffith



0
Comment actions Permalink


My telemetry more-or-less showed that inspections that resolved a lot of items (all variables, all method calls) took about ten times as long as inspections that didn't do resolutions. Now I will say that telemetry showed that my inspections weren't taking all that long, with a couple of pathological exceptions. So we're both right. That said, my performance review comment of putting off all possible resolve() calls until you're absolutely sure you need 'em still stands.

--Dave Griffith

0
Comment actions Permalink

So, in order to
- delay resolve() calls as much as possible, as suggested by Dave,and
- process
++foo;
assignments too, as corrected by Alexei,
I turned to:

public void visitReferenceExpression (PsiReferenceExpression
expression)
{
if (! PsiUtil.isAccessedForWriting (expression))
return;

PsiElement element = expression.resolve ();
if (element instanceof PsiField) {
PsiField field = ((PsiField) element);
boolean isAnnotated = field.getModifierList
().findAnnotation (TRACKED_ANNOTATION) != null;
if ( isAnnotated
&& accessedOutsideTheClass (expression, field))
{
registerProblem (io_problemList, expression,
manager);
return;
}
}
super.visitReferenceExpression (expression);
}

Alain

0
Comment actions Permalink

I don't think you should use @ReadOnly because as you said it means
something totally different than "cannot write outside containing class"
to most people.


Alain Ravet wrote:

Alexey

>> I'd also rename @AlmostFinal to @ReadOnly.
>>
>>


It's much better. Actually, I even find it too good for my modest
inspection support.
I feel like stealing a universally acclaimed and understood word for a
very annecdotical and limited purpose.

What do you think?

Alain

0
Comment actions Permalink

Keith Lea wrote:

I don't think you should use @ReadOnly because as you said





One more reason: the typical use case is

step 1:

...
final int i;
final int j;
...


step 2:

...
@AlmostFinal int i;
final int j;
...



In that case, @Final would make sense too but I feel that, once again,
it looks too official for my little inspection.

Alain

0
Comment actions Permalink

One more thing. Your solution as given will double-report issues in inner classes (anonymous or not) contained in methods. You need to add a visitClass() method to your visitor which prevents it from drilling down into a class if it's already in a class.

0
Comment actions Permalink

I would prefer something more descriptive like @PrivateWrite or
@PublicReadOnly, now that I've thought about it some more.

Alain Ravet wrote:

Keith Lea wrote:

>> I don't think you should use @ReadOnly because as you said





One more reason: the typical use case is

step 1:

...
final int i; final int j; ...


step 2:

...
@AlmostFinal int i; final int j; ...



In that case, @Final would make sense too but I feel that, once again,
it looks too official for my little inspection.

Alain

0
Comment actions Permalink

Keith Lea wrote:

I would prefer something more descriptive like @PrivateWrite or

>

I like this one.

Alain

0
Comment actions Permalink

Dave

>One more thing. Your solution as given will double-report issues in inner classes
>

Thanks.

Alain

0
Comment actions Permalink

Alain Ravet wrote:

Keith Lea wrote:

>> I would prefer something more descriptive like @PrivateWrite or


I like this one.

Alain


How about @PublicFinal, so you just change 'final' to '@PublicFinal'.

--
Rob Harwood
Software Developer
JetBrains Inc.
http://www.jetbrains.com
"Develop with pleasure!"

0
Comment actions Permalink

Rob

>
>>> I would prefer something more descriptive like @PrivateWrite or
>>
>>
>> I like this one.
>>


I had to chose a name, so I went with this one. and published the plugin
yesterday:
http://intellij.org/twiki/bin/view/Main/PrivateWriteInspectionPlugin



How about @PublicFinal, so you just change 'final' to '@PublicFinal'.

>

Well, the field you are watching could be protected, package, or even
private (=> to prevent modification by an inner class).
I may add synonym annotations - like your suggestion, and @AlmostFinal-,
in a future release, but I fear it would add confusion.

Alain

0

Please sign in to leave a comment.