Bug 231396 - Refactoring AspectJ
Summary: Refactoring AspectJ
Status: RESOLVED FIXED
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: 1.6.0   Edit
Hardware: PC Windows Vista
: P3 normal (vote)
Target Milestone: 1.6.1   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-09 18:10 EDT by Andrew Clement CLA
Modified: 2008-06-19 14:13 EDT (History)
1 user (show)

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-05-09 18:10:06 EDT
Under this enhancement I'm going to cover all the refactoring work I'm doing for 1.6.1 of AspectJ.  The aims are:

- reduced disk footprint (smaller weaver)
- reduced garbage creation at weave time
- reduced memory usage whilst weaving
- faster weaving (primarily loadtime and binary weaving, possibly also benefiting compile time)
Comment 1 Andrew Clement CLA 2008-05-09 19:24:17 EDT
Some classes get included in aspectjweaver.jar even thought they are only required by the compiler.  Moving them between projects will help reduce the size of aspectjweaver.jar.  First: ConfigParser
Comment 2 Andrew Clement CLA 2008-05-12 14:01:29 EDT
AjTypeImpl - won't hunt for particular annotations if there are none at all
BetaException - removed
CollectionUtil - removed
CharOperation - moved
NameConvertor - moved
Comment 3 Andrew Clement CLA 2008-05-13 13:45:33 EDT
Removed WeaverMetrics.

Changed UnwovenClassFile to have a char[] name and use that where we can, drastically improves binary weave time on a large jar.
Comment 4 Andrew Clement CLA 2008-05-28 19:55:19 EDT
I have tagged the codebase with v1_6_1x as I am about to check in a huge refactoring. 

500 code changes

Rough list of changes in this commit:
- split bcel into a verifier and non-verifier related set of classes, we don't package the verifier now
- restructured bcel to reduce the number of classes in it for representing instructions
- restructured bcel to reduce need to transform to and from types (like ConstantPool to ConstantPoolGen)
- introduced bitflags into BcelMethod rather than lots of booleans (work in progress)
- promoted fields to be first class entities in LazyClassGen so they are treated the same as methods (it is good to be consistent)
- removed unused code from all over
- removed passing of an xref handler when ltw - prevents unnecessary calls to a handler that does nothing
- removed ability to use heavyweight linenumber/localvariable gens in the weaver, now just uses tags (it used to be optional)
- reduced unnecessary copies of the strings arg0-argN

Observations from this so far:
weaver jar is ~25% smaller (from ~1.9M to ~1.5M)
weaver jar contains ~300 less classes (25% less classes)
in memory weaver objects (for my benchmark) have reduced in size by 400k (were 2.9M, now 2.5M, more still to do here)

More optimizations to follow.  I have tested this as thoroughly as I can, but now I need to get it out for people to try.  I can rollback to the tag if serious problems are found, but I'd like this to be the base moving forward.
Comment 5 Eugene Kuleshov CLA 2008-05-29 00:26:50 EDT
Andy, would you consider to completely move from BSEL to ASM?
Comment 6 Andrew Clement CLA 2008-05-29 16:19:30 EDT
I might consider moving at some point, but not right now.

---

I had to revert one change I made, although it saved memory it caused tests relating to intertype declarations and annotations to fail.  I have since reworked the change and it is back in again.  This was the code in LazyClassGen to try and use the existing method object rather than build a duplicate.  It looks like that duplicate was having a lot of state attached to it which was then discarded (correctly) when the duplicate was ditched - when I changed the code to avoid duplication I was also holding onto this extra state.  I now clear it in a different way, so we still avoid duplication.

More changes that are going in today:
- Code moved out of the mainline into the test tree.  stuff that is only ever used within the test cases! ZipFileWeaver/MemberImpl.fieldFromString/MemberImpl.methodFromString
- changed ctor of UnwovenClassFileWithThirdPartyManagedByteCode so that it can take a classname, this means downstream it doesn't have to reparse any bytecode to work it out
- BcelAdvice shrinks the state it holds on to if it isn't for around advice

Next to tackle is the member hierarchy which has too much stuff at the wrong levels, leading to numerous objects which have null field values.  Those fields should instead be on an object lower in the hierarchy.
Comment 7 Andrew Clement CLA 2008-06-04 14:11:05 EDT
I've moved some of the annotation related methods down the Member hierarchy - still more to do here.

I've also merged in the change to improve woven method packing.  We previously created a lot of unnecessary garbage when packing up a woven method because we packed it into a new Method entity (copying lots of instructions/localvars/linenumber entries).  i've changed it so that when possible we do a 'local packing' into the current method entity - this works in cases where we don't need the extra information in the local method copy (where the shadows are/etc) so works for ordinary methods that are woven.  It cannot be done for advice since it overwrites some state we need to maintain.  however, given that 99% of methods are not advice, it helps a lot on cutting down garbage creation.  This behaviour defaults to on but can be turned off with:
-Xset:fastPackMethods=false

Besides that change, the last remaining large scale change I want to get into 1.6.1 if possible is the shrinking of weavers over time when their classloaders are not actively being used (but have not been discarded).  There is discussion on this change here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=227484 - but it will not get committed until more testing has been done.

I will continue to address the OPTIMIZE task tags I've been putting throughout the code as I go along, but hopefully there are no other  drastic changes going in.
Comment 8 Andrew Clement CLA 2008-06-04 17:31:27 EDT
Addressed a few OPTIMIZE tags, also ran PMD over the code.  It found a real problem in a method in the weaver Utility class.  A switch with no default was causing us to not perform a bytecode optimization.  For low number constants (-1>4) we should have been using the ICONST instructions but due to this bug after we worked out the right instruction we then 'forgot' and went back to using BIPUSH.  ICONST is just one byte but the BIPUSH are two bytes each.  This had a knock on effect that some of our tests that compare bytecode output against expected had to have their expected output changed.
Comment 9 Andrew Clement CLA 2008-06-11 17:17:49 EDT
Done everything I plan to do now for 1.6.1 - need the dust to settle a bit.  I'll publish some numbers for the benefit of refactoring in a new and noteworthy attached to 1.6.1.

The change in:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=227484

will have to wait.
Comment 10 Andrew Clement CLA 2008-06-19 14:13:55 EDT
After some more performance analysis on the hotspots in the code, I'm committing a few more changes:

- caching in ConstantPool for a couple of types of entry
- caching member created in PerSingleton perclause
- reducing calls to toArray