[aspectj-dev] issues with "advice weaving in aspectj"

["Erik Hilsdale" <eh-ajdev@xxxxxxxxxxxxx>]

I just had a short email flurry with Shay Cohen about some weirdnesses in
Jim and my implementation paper, and I thought the questions and answers
might be of general interest.

-erik

------------------
Let me try to answer your implementation questions:

Why do we generate redundant load/store pairs?

    Our general architecture is to do a "defensive copy" of all state
    exposed to the advice -- that is, we stick the value in a temporary
    variable and then only refer to the state in a temporary variable.
    You're right, this is required for after advice, for example:

    void foo(int i) {
        i = 37;
    }

    after(int j) returning: execution(void foo(*)) && args(j) {
        System.err.println(j);
    }

    The correct behavior for this bit of after advice is to print
    the _original_ parameter.  If we didn't do a defensive copy, 
    we would print 37. 

    Of course, we didn't need to do a defensive copy for the paper's
    example.  But our compiler considers removing that defensive copy
    to be an optimization, and we just haven't done that optimization
    yet since it doesn't seem to be a real problem in practice.  We could
    have just removed the defensive copy from the paper, but we thought
    that would have been intellectually dishonest.  So instead you
    get to see a paper about a real object of engineering in action 
    *smile*.
 
Why insert a checkcast[String] if it's guaranteed to succeed?

    Because Java requires us to do so.  Though _we_ know
    that the checkcast is guaranteed to succeed, the JVM requires
    we insert a checkcast bytecode before we invoke a method
    defined on String that isn't defined on Object.  You'll see
    this bytecode pattern often with generic methods, for example.
    Consider this to be like the Java code:

    void go(Object o) {
        if (o instanceof String) {
            System.err.println(
                ((String) o)  // ****
                .length()); 
        }
    }

    if the cast to (String) on line **** were not present, 
    the Java compiler would complain.

Would you mind if I sent a copy of this message to the aspectj-dev 
mailing list?  I think your questions are of general interest.

-erik


-----Original Message-----
From: Shay Cohen [mailto:shailu@xxxxxxxxxxxxxxxx] 
Sent: Wednesday, June 02, 2004 10:21am
To: hilsdale@xxxxxxxx
Subject: Advice Weaving in AspectJ


Hi

First thanks again for the help.
Second, I have a few questions that still intrigue me, I have already
presented my supervisor with those questions and we still couldn't found an
answer. I will be more than glad if you could give me a little help with
them.

1. In the section "2.2 Bytecode transformation" you show there the following
byte code:

void go(java/lang/Object):
0: aload_1 # copy first argument into
1: astore_2 # a temporary frame location
2: aload_2 # check whether argument
3: instanceof [String] # is actually a String
6: ifeq 19 # if not, skip advice
9: invokestatic [A.aspectOf] # get aspect
12: aload_2 # load argument again
13: checkcast [String] # guaranteed to succeed
16: invokevirtual [A.ajc$before$A$a3] # run advice
19: return

I could not figure why the astore_2 (and a aload_2) is done? (I could have
understood it in a case for an "after join point", but not for "before join
point"). 
Moreover, the checkcast[String] seems redundant since it's guarantied to
success?

2. In section "6. Compile time performance" you have compared the AspectJ
compiler results to Sun's Javac compiler results. wouldn't it have seemed
more logical to first compare it to the base compiler of AspectJ which was
taken from Eclipse.org (as written is the paper) ?

Thanks in advance 
Shay Cohen