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 }
Please sign in to leave a comment.
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
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...
>
>
>
>
>
>
>
>
>
>
>
>
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
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:
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
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
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...
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.
>
>
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
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
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:
>> I'd also rename @AlmostFinal to @ReadOnly.
>>
>>
Keith Lea wrote:
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
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.
I would prefer something more descriptive like @PrivateWrite or
@PublicReadOnly, now that I've thought about it some more.
Alain Ravet wrote:
>> I don't think you should use @ReadOnly because as you said
Keith Lea wrote:
>
I like this one.
Alain
Dave
>One more thing. Your solution as given will double-report issues in inner classes
>
Thanks.
Alain
Alain Ravet wrote:
>> I would prefer something more descriptive like @PrivateWrite or
How about @PublicFinal, so you just change 'final' to '@PublicFinal'.
--
Rob Harwood
Software Developer
JetBrains Inc.
http://www.jetbrains.com
"Develop with pleasure!"
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
>
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