Bug 140695 - Use LTWWorld not BcelWorld for load-time weaving
Summary: Use LTWWorld not BcelWorld for load-time weaving
Status: RESOLVED FIXED
Alias: None
Product: AspectJ
Classification: Tools
Component: LTWeaving (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-05-08 17:21 EDT by Ron Bodkin CLA
Modified: 2012-04-03 15:26 EDT (History)
1 user (show)

See Also:


Attachments
patch to loadtime module (1.32 KB, patch)
2006-05-08 17:21 EDT, Ron Bodkin CLA
aclement: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ron Bodkin CLA 2006-05-08 17:21:23 EDT
Please update AspectJ load-time weaving to use the LTWWorld instead of the BcelWorld, to take advantage of the load-time weaving optimizations it provides. Here is a patch for doing that. A possible alternative would be to add an -Xset: option to allow disabling the use of LTWWorld in case there is a problem...
Comment 1 Ron Bodkin CLA 2006-05-08 17:21:50 EDT
Created attachment 40654 [details]
patch to loadtime module
Comment 2 Ron Bodkin CLA 2006-06-06 10:24:23 EDT
This has been integrated in recent dev builds. 
Comment 3 Andrew Clement CLA 2006-06-06 10:55:25 EDT
naughty!!!!!!!!!!!

this bug was deliberately left open because we have yet to have a successful build with that change in.  As such this change is not available in a published dev build yet.  If the build doesn't succeed soon, I'll have to roll it back again.
Comment 4 Ron Bodkin CLA 2006-06-06 11:18:29 EDT
If you have to roll it back, could you at least leave a -Xset option to enable the LTWWorld for those who would like the performance boost?
Comment 5 Andrew Clement CLA 2006-06-06 11:25:03 EDT
I don't plan to start using Xset for 'things I dont understand' ...  I'm hopeful the next build will succeed.
Comment 6 Matthew Webster CLA 2006-06-08 11:22:55 EDT
Unfortunately this breaks under OSGi due to the rather cavalier use of Class.forName in the weaver. This problem is exacerbated by the behaviour of the harness noted in Bug 117885. Let me explain. 

In general there are 3 namespaces to consider: the application, the AspectJ runtime on which the woven application depends (for NoAspectBoundException at least) and the weaver. The weaver also depends on the runtime but the application does and should not depend on the weaver (otherwise the user would need to add it to the classpath in order to run their application). When using Class.forName() the weaver can use the application class loader to resolve application types and its own loader to resolve extensions like Java15ReflectionBasedReferenceTypeDelegate but _not_ vice versa. Using its own loader (i.e. Class.forName(String)) to resolve application types was the cause of  Bug 116229 and Bug 116254. Using the application loader (in ReflectionBasedReferenceTypeDelegateFactory. create15Delegate()) to load extensions is the cause of this problem. The harness did not help to catch this because currently when it runs an XML-based test it creates a new class loader with its ownclasspath and throws everything into it including stuff that wasn’t specified explicitly by the user like weaver5. Also in traditional class loader hierarchies the application class loader _may_ delegate to the one for AspectJ. Not under OSGi.

I suggest we do the following:
1. Apply the changes to the harness suggested in Bug 117885. We may want to go further by creating a sandbox namespace that _only_ depends on the AspectJ runtime but this will need some further investigation.
2. Change ReflectionBasedReferenceTypeDelegateFactory to use the weaver class loader. We may also like to discourage (read remove) the direct use of Class.forName() in the weaver. One approach would be to add World.forName() and World.forApplicationName() methods which abstract the correct usage.
3. Ensure the user specifies the correct classpath which remains constant throughout the test run. This is not a problem for the “run-all-junit-tests” project which depends on weaver5 etc but if individual tests in the “tests” project are executed they will fail. The answer is for the user to edit the classpath for their launch configuration (we may want to put out a helpful warning in this respect).
Comment 7 Matthew Webster CLA 2006-06-13 09:59:15 EDT
1. Changed ReflectionBasedReferenceTypeDelegateFactory to use the weaver class
loader. 
2. Changed AjcTestCase.run() to build completely new classloader hierarchy to include XXX5 modules on weaver classpath while isolating application in its own sandbox.
3. Defer changes to prevent misuse of Class.forName() to Bug 117885.
Comment 8 Matthew Webster CLA 2006-06-14 08:05:07 EDT
Fix available
Comment 9 Matthew Webster CLA 2006-06-14 09:43:40 EDT
This one just won't lie down! I get java.lang.NoClassDefFoundError: org/aspectj/asm/IProgramElement running compiler tests. This is because the asm project is missing from AjcTestCase.DEFAULT_CLASSPATH_ENTRIES. This error did not occur on the build machine because it uses "aj-build/jars" or in my original Eclipse for the same reason (I had a release build lying around).

I have added asm to the list. A better approach would be to copy the URLs from CLASSPATH. An even better approacj would be to get rid of DEFAULT_CLASSPATH_ENTRIES!
Comment 10 Matthew Webster CLA 2006-07-05 06:14:58 EDT
Fix available.