Bug 246021 - FindBugs reporting another optimization
Summary: FindBugs reporting another optimization
Status: RESOLVED FIXED
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: DEVELOPMENT   Edit
Hardware: PC Windows XP
: P3 enhancement (vote)
Target Milestone: 1.6.2   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-09-02 18:22 EDT by Andrew Clement CLA
Modified: 2008-09-02 20:29 EDT (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Clement CLA 2008-09-02 18:22:57 EDT
Ben Hale reported that FindBugs was producing a warning about a dead store to a local variable (a variable that is never then read within the method).  This bug is to investigate and hopefully remove the dead store.
Comment 1 Andrew Clement CLA 2008-09-02 19:58:35 EDT
Whenever a join point shadow is advised by some advice, we look at the advice and see if it uses any of the available arguments exposed by the join point.  In a simple case, for example, the arguments for the method-execution join point are those being passed to the method.  If any arguments are used by the advice, then we copy all of them to other local variables and use those copies as arguments to the advice call.  We copy them because we do not want any changes made by another piece of advice at the same join point to affect what our advice sees. 

Here is an example:

aspect X {
  before(Object o): execution(* foo(*,*)) && args(*,o) {
  }

  public void foo(String s, StringBuffer t) { }
}

So the second parameter at the execution join point is exposed and passed to the advice.  The bytecode for foo() looks like this:

  Code:
   Stack=2, Locals=5, Args_size=3
   0:   aload_1
   1:   astore_3
   2:   aload_2
   3:   astore  4
   5:   invokestatic    #58; //Method aspectOf:()LX;
   8:   aload   4
   10:  invokevirtual   #60; //Method ajc$before$X$1$fb064c60:(LObject;)V
   13:  return

The pairs of calls "aload_1/astore 3" and "aload_2/astore 4" copy the method parameter values into local variables 3 and 4.  Then variable 4 is used as the advice parameter (see instruction 8, 'aload 4').

Currently all the arguments are initialized and copied in this way regardless of whether they are used by the advice.  A tool being picky over the above valid code would say that the 'astore_3' is a waste since it is never used - and so consequently the 'aload_1' is a waste as well.  That is what FindBugs is reporting.

AspectJ has done this since the beginning of time.  However, there is clearly scope for some optimization to copy only the arguments that the advice needs.  This is not entirely trivial as the argument setup code is a little divorced from the advice invocation code.

However, I've now implemented it.  Using the new code, if I compile the program above I get a new version of the foo() method:

  Code:
   Stack=2, Locals=4, Args_size=3
   0:   aload_2
   1:   astore_3
   2:   invokestatic    #58; //Method aspectOf:()LX;
   5:   aload_3
   6:   invokevirtual   #60; //Method ajc$before$X$1$fb064c60:(LObject;)V
   9:   return

where only the argument used is copied.


Comment 2 Andrew Clement CLA 2008-09-02 20:29:45 EDT
fix committed. should generate much neater code in quite a few situations.