Bug 75442 - The compiler generates unnecassary instance variables w/ perthis association
Summary: The compiler generates unnecassary instance variables w/ perthis association
Status: RESOLVED FIXED
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: 1.2   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 1.5.0 M3   Edit
Assignee: Alexandre Vasseur CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-10-01 06:49 EDT by Antti Karanta CLA
Modified: 2005-09-27 09:28 EDT (History)
0 users

See Also:


Attachments
an example project where the error occurs (10.78 KB, application/zip)
2004-10-01 06:50 EDT, Antti Karanta CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Karanta CLA 2004-10-01 06:49:23 EDT
When compiling the included example aj project, the compiled class AnotherClass.
class has instance variables generated into it even though it has nothing to do 
with the aspect. This generates unnecessary memory consumption and noise e.g. 
when debugging.
Comment 1 Antti Karanta CLA 2004-10-01 06:50:48 EDT
Created attachment 14947 [details]
an example project where the error occurs
Comment 2 Alexandre Vasseur CLA 2005-02-24 09:35:20 EST
what is the status ?

Some note to remember that:
It is affecting @AJ aspects and LTW in general, since *ALL* class gets touched.
Right now avoid touching @AJ aspects.
How the LTW (be it LTW for Java 1.4 or LTW for WebLogic-aspect ""works"" with
such a behavior ?


Comment 3 Andrew Clement CLA 2005-02-24 09:56:44 EST
the status is that I have a prototype fix in my workspace that seems to hang
together - but I want to add a couple more testcases for some more sophisticated
scenarios.
Comment 4 Adrian Colyer CLA 2005-03-23 08:42:33 EST
we should resolve this one way or the other by m4 timeframe if not before....
Comment 5 Alexandre Vasseur CLA 2005-05-04 04:15:52 EDT
in LTW it happens that all classes get the ajc$MightHaveAspect interface, and
thus the serializable classes without a serialVerUID triggers a warning:

warning serialVersionUID of type junit.framework.ComparisonFailure needs to be
set because of added interface
ataspectj.PerClauseTest$TestAspectPerTarget$ajcMightHaveAspect
[Xlint:needsSerialVersionUIDField]
warning serialVersionUID of type junit.framework.AssertionFailedError needs to
be set because of added interface
ataspectj.PerClauseTest$TestAspectPerTarget$ajcMightHaveAspect
[Xlint:needsSerialVersionUIDField]



Comment 6 Alexandre Vasseur CLA 2005-05-09 06:14:05 EDT
ok I have a fix for it right now but that's rather dirty... here is my notes.
Andy please comment.

It seems that what we d 'like is match and see if we may have a chance to be the
type for which there might be a shadow where this perObject aspect applies. This
match can't be guessed easily (and can't be a narrow "may be" easily neither).

So my fix is using a reverse approach. For each *shadow munging* that occurs, if
the advice is for a perObject aspect, I am registering the shadow perX type
(target or this, depends of the aspect) in a hashmap.
At the end of the shadow munging for a .class file, I thus have a map of all
aspect that did shadow munging for the type (wondering if this structure is
somewhere else by the way - Andy can refactor that later anyway)

Then what we want is query this hashmap when we apply the
perObjectInterfaceTypeMunger so that the ajcMightHaveAspectXXX is applied to the
only type that needs it (especially sensitive in LTW).
The issue there is that type munging (even such "system level" type munging
happens before shadow munging in the weaver.
So we need sort of "post shadow munging type munger". To do so asap without too
much change all around, I added a "postMunge(..)" method in the BcelTypeMunger,
and remove the if part in "munge(..)" for the PerObjectInterface munger.
Then I call the "postMunge" from the weaver.

The last trick is that since type mungers are gathered on a type basis before
shadow munging, I cannot use those (since before type munging there is no stuff
in the hashmap that will help us do the match).
So I pass thru the *entire* list of type munger, and call postMunge() on all of
them. The postMunge method does only do something for the PerObjectInterface
munger (actually that d be better with a postMatch + postMunge) for better symetry.

I can commit the stuff.
comments Andy ?

Comment 7 Andrew Clement CLA 2005-05-09 06:51:57 EDT
Hi Alex - yes I think that was the kind of conclusion I was coming to as well -
we don't know enough at normal type munge phase to say one way or the other,
we'd ideally like to postpone this kind of munge until later on.

I didn't write down my findings last time I was looking into this (i'm an idiot)
so its taking a while to page it back in to my brain.  I think your approach is
reasonable - we do maintain a list of the aspects that affected a type but only
when in reweavable mode which isnt the default (yet).

I'm nervous about introducing further phases to the munging, but I can see that
for what you call 'system level' mungers it is probably the only way to go.  I
think I'd like Adrian to comment on this as it is a serious change to the pipeline.

It would be nice to tag these special ones so that they are ignored on first
pass and only dealt with in second pass - rather than you iterating over the
full list each time and ignoring particular ones at particular times.

I look forward to the performance improvement we get for per*** aspect weaving
in batch mode as well as LTW.

can you wait for Adrian to comment before committing?
Comment 8 Alexandre Vasseur CLA 2005-05-09 10:50:23 EDT
i ll wait. I am refactoring as you describe here as well. The best we should do
is pbly go down to 
inAspect.crosscuttingMembers.addTypeMunger(..)
and same with CrosscuttingMembersSet + BcelWeaver 		typeMungerList =
xcutSet.getTypeMungers();

so that we have this "lateTypeMunger" concept all around in a clean way, where
match() on those is deffered after shadow munging.
Comment 9 Alexandre Vasseur CLA 2005-05-09 11:11:23 EDT
question: should the lateTypeMungers surface in
ResolvedTypeX.getInterTypeMungers (or other getxxxx to be added there).

If yes, why and what for, considering that no weave phase comes after, we don't
really care about which lateTypeMunger did **actually** applied to the
resolvedTypeX (or do we for tooling etc ?)
Comment 10 Alexandre Vasseur CLA 2005-05-10 11:51:50 EDT
ok fixed and committed.

If you want to see how it looks, a good starting point is
PerObjectInterfaceTypeMunger and look for calls to registerAsAdvisedBy and
unregisterAsAdvisedBy to see respectively perObject handling and lateTypeMunging 

The part I don't like is this registerAsAdvisedBy which is redundant with data
kept when in reweavable mode (and that is further specific for perObject
Munger), and aside costs 2 hashmap lookups.

No test for it (should we compare bytecode to bytecode things??) but implicitly
covers all around I suppose.

Note: resolvedTypeX does not expose its lateMunger for now.
Comment 11 Alexandre Vasseur CLA 2005-05-12 09:21:02 EDT
ouch. One test case make us realize we are wrong, me first :-(

see test "inheritance, around advice and abstract pointcuts [eachobject] (still)"

when it fails: type C is weaved but we had call side pointcuts
occuring in OverridingPointcuts that will (but it will be too late)
update some state in the perObject lateTypeMunger to say "we had a
match for this advice of a perObject aspect, so please remember that
so that the target/this type is added this ajc$MightHaveAspect
interface when you 'll later see it".

Ouch, despite what we discussed on Bugzilla I think it actually just
can't work like that. It is not tied to the test that has some inner
classes. It is tied to perTarget and call side advice, for which we
may have weaved the target type already while we don't have weave the
(at least one) caller.
Ie when it fails the target type does not has the X$ajcmightHaveAspect
interface since it has already gone thru the weaver

I ll rollback the change (4 lines) for now until we manage to fix this
#75442. It means we need to back off on a early matching in the
pointcut matching (but fine grained enough to avoid 75442).
Comment 12 Alexandre Vasseur CLA 2005-05-12 09:47:54 EDT
note: i have rollback the perObject things (4 lines make the change).

Off course, LTW perObject test fails since the X$ajcMightHave.. interface is
added all around (huge cost for LTW) and when a serializable class is
encountered, a warning is emitted :
so I turn off LTW tests from AllTestsAspectJ150 ...



     [java] WeavingAdaptor.weaveClass junit/framework/Assert
     [java] WeavingAdaptor.weaveClass junit/framework/ComparisonFailure
     [java] warning serialVersionUID of type junit.framework.ComparisonFailure
needs to be set because of added interface
ataspectj.PerClauseTestAspects$TestAspectPerTarget$ajcMightHaveAspect
[Xlint:needsSerialVersionUIDField]

BUILD FAILED
junit.framework.AssertionFailedError: test "AjcLTW PerClauseTest -XnoWeave"
failed: failure in 'ltw.PerClauseTest'
D:\aw\cvs_aj\org.aspectj-HEAD\modules\tests\java5\ataspectj\ajc-ant.xml:26: Java
returned: -1


Comment 13 Alexandre Vasseur CLA 2005-06-07 04:13:59 EDT
commited new impl based on visitor - should be fixed in next dev build
Comment 14 Alexandre Vasseur CLA 2005-07-04 10:46:44 EDT
close when M3 ship
Comment 15 Alexandre Vasseur CLA 2005-07-04 12:07:17 EDT
remind
Comment 16 Alexandre Vasseur CLA 2005-07-04 12:07:34 EDT
close with M3
Comment 17 Alexandre Vasseur CLA 2005-09-27 09:23:02 EDT
was M3 remind
Comment 18 Alexandre Vasseur CLA 2005-09-27 09:24:27 EDT
was M3 remind
Comment 19 Alexandre Vasseur CLA 2005-09-27 09:28:38 EDT
was M3 remind