Community
Participate
Working Groups
Often, there is a need to keep per-join point (static part, usually) information. While using hashmaps works, it is inefficient in many situations. While perJoinPoint and perJoinPartStaticPart aspect associations might work, I have a simpler proposal to address the need. I will like to add the following APIs: void JoinPoint.StaticPart.setUserObject(Object) Object JoinPoint.StaticPart.getUserObject() And, for symmetry, void JoinPoint.setUserObject(Object) Object JoinPoint.getUserObject() Then I will be able to store profile information as follows: Object around() : profiled() { long startTime = getTime(); Object ret = proceed(); long endTime = getTime(); ProfileInfo jpStats = (ProfileInfo)thisJoinPointStaticPart.getUserObject(); if(jpStats == null) { thisJoinPointStaticPart.setUserObject(new ProfileInfo()); } jpStats.record(endTime-startTime); }
We had such a concept in AW Not just an object but a map so that more than one object can be associated to the jp. Note that the StaticPart version will not be thread safe. Given your sample, i expect that the "record" method is doing the locks etc.. Further on, what would happen with inlining (when there is no closure).
(In reply to comment #1) I think the default implementation should provide no thread safety. User can, if warranted, make such access thread safe by either: - Explicitely taking locks around get/setUserObject and/or accessing/modifying the user object. - Using a user object of type that is thread safe (HashTable, for example). In this case, get/setUserObject worn't be thread-safe, but accessing and modigying the underlying user object will be. Re: Inlining Even when inlined, thisJoinPoint and thisJoinPointStaticObjects are created. Right? If so, inlining should pose no problem.
We're not going to get to this in AJ 1.5.0. Marking as "LATER" for consideration in 1.5.1 and future release planning.
I would really like to have this reconsidered for 1.5.1 ... it is motivated by the same kind of efficiency concerns as pertypewithin aspects, e.g., where you want to cache information and calculate data at each shadow advised (e.g., for monitoring, error handling, etc.). JBoss AOP provided a perjoinpoint aspect instantiation model to address the same goals...
I have had some thoughts on this. The user object could be kept in the StaticJoinPoint instance. However you need one for each aspect otherwise you cannot rely upon it. There could be one object per aspect, not aspect instance, which would be very efficient. However we need to make per-aspect access transparent and possibly protected to prevent inadvertent corruption. One solution is to allocate a user object array in the StaticJoinPoint instance with one entry for each aspect that affects the target class. The index is the implied or defined aspect precedence for that class i.e. the one with the highest precedence is 0. The object is accessed from advice using thisJoinPoint.get/setUserObect(). The transient JoinPoint object passed to the advice is instantiated with the aspect index. The downside with this approach is the need to us thisJoinPoint. A simple implementation would allocate a user object for each aspect declared to the weaver but this would cause unnecessary bloat to StaticPartImpl instances which we have recently streamlined to reduce heap footprint in Bug 59076 “Reduce footprint of JoinPoint.StaticPart objects”. An optimization would only allocate user objects for join points whose associated advice actually used the feature (in a similar way to detecting static only access to thisJoinPoint). A further enhancement would use a StaticPartImpl subclass for the new “userObects” array reference to save space when the feature is not used at all. I will post a patch to runtime when I have something working.
The reason I like this approach is that thisJoinPoint is used as a localized, transient “ticket” granting an aspect access to its data. The user data has the same scope as thisJoinPointStatic part i.e. it can only be access in advice but passed to others if necessary. I would like to make access harder by avoiding changes to the public JoinPoint.StaticPart interface. One option is a UserObjectHolder a reference to which is passed to a JoinPoint on construction. Implementation ideas with increasing levels of performance (and complexity): 1. Allocate an Object[] in StaticPartImpl whose size is the total number of aspects. Bloated. 2. Allocate an Object[] whose size is the number of aspects affecting the class i.e. 0-N where N is the total number of aspects. This is more complex as the aspect # would be different for each class. 3. Allocate an Object[] whose size is the number of aspects affecting the class iff at least on of them uses get/setUserObject(). Probably the sweet spot. 4. Allocate an Object[] whose size is the number of aspects affecting the class iff advice associated with the join point uses get/setUserObject(). 5. Allocate an Object[] whose size is the number of aspects that have advice which is associated with the join point and uses get/setUserObject(). This is even more complex as the aspect # would be different for each join point in a given class. 6. Allocate a StaticPartImpl subclass for those join points whose associated advice using get/setUserObject(). This would mean zero cost is systems where the feature is not used. What about abstract aspects, do they get their own slot or do they share it with sub-aspects? Can we think of an efficient implementation that would allow a user object per aspect instance or is the current model more appropriate?
Renewed interest.
review this for 1.6.0
I'm thinking about implementing this but am having a few problems. Currently thisJoinPointStaticParts have no knowledge of the aspect that caused them to come into existence. They are shared amongst any advice that needs them at a particular shadow in the code. The initial thought would be: public void setUserObject(Object data) {} public Object getUserObject() {} But, as discussed in this bug, that breaks down because there are multiple aspects around, you might get back something that another aspect decided to store against the jp object. I don't know how you would generate an int id for an aspect that was robust across compile time weaving and later possible reweaving with a different set of aspects. So, let's change the methods to store against a key - reducing the collision chance because differing aspects are unlikely to be using the same keys: public void setUserObject(Object key, Object value) {} public Object getUserObject(Object key) {} But where is the data stored? Well each staticpart object holds a map instance - that would unfortunately increase the tjp object size by 4bytes (from 12 to 16 - which is quite a lot when you have a tracing aspect using 1000s of these things). And what type of map would it be to avoid problems with concurrent thread access? So we could introduce a new kind of jp to address the size issue - StatefulStaticPart (as outlined in (6) in the previous comment). But that means throughout the whole codebase we have to modify all the code dealing with joinpoints to do the right thing with this new kind of jp, and that is a big piece of work. I'm hoping there is a much simpler way... but can't quite see it at the moment.
I understand the motivation for changing to a map, but it almost completely invalidates my desire for this feature: efficiency. The only change to my current aspect would be not creating my own map, but using this one instead. The scenario I'm working in is: * use an annotation on a field (perhaps multiple per class) * use an aspect to 1) associate a java.util.concurrent.atomic.AtomicMarkableReference with each field, 2) around get for these fields atomically check the markable reference (and do stuff). The aspect I've already implemented is constructing a ConcurrentHashMap to map from field to AtomicMarkableReference. It's lookups in this map that slow down everything considerably. (If you're curious see http://code.google.com/p/dash-framework/source/browse/trunk/dash-obtain/src/dash/obtain/aspects/ObtainAccess.aj) The two solutions I see are: 1) (Somehow) implement the original setUserObject/getUserObject signtatures to be array indexes or field access for efficiency. 2) Extend the aspect instantiation model to include perfield. I don't know how to do that somehow part either, that why I voted for this bug! ;)
Hi, we are working on an in-house monitoring tool utilizing aspectj and came across the same issue. What we want to achieve is to provide an annotation based API which can be used to identify methods in the code which needs to be measured. With annotations we can fully decouple the capturing part from the implementation part. Ideally the code would be weaved in in production applications and monitoring can be turned on/off at runtime on demand. If i weren't use aspectj the following code would be written: class A{ StopWatch stopWatch=...; void methodNeedsToBeMeasured(){ stopWatch.start(); //doStuff stopWatch.stop(); } } With the current AspectJ implementation this is the nearest i can get: class A{ //Injecting this field to the class through ITD Map<String,StopWatch> signatureToStopWatchMap; void methodNeedsToBeMeasured(){ StopWatch stopWatch= // lookup the current stopwatch from the map based on the signature name passed in to the JoinPoint stopWatch.start(); //doStuff() stopWatch.stop(); } } The lookup in the map slows down things considerably(like mentioned in this Jira issue) and makes us thinking whether this slow down is acceptable in a production environment which questions the usefulness of the annotation API. I would like to ask if this Jira is considered to be fixed in the foreseeable future, given that there is another use case which requires it ;-) I had a quick look at JBoss AOP which has implemented this feature by providing a per joinpoint instantiation model and wonder why this has been missed out from Aspectj. I have 2 guesses here: a) because JBoss aop is proxy based - as far as i know- b) instantiating an aspect at every join point has a bad performance impact In any case i would like to here your opinion about this. Thanks you for your time and answer in advance! Kind Regards, Zoltan Szel
Hi Zoltan, As discussed in comment 9, it is currently blocked on trying to come up with a design that is not simply a map internally as that will have the same performance issue as maintaining your own map.
let me put some brain power into seeing if there is a sensible design that we could use in the short term.
Achieving a performant implementation of perjoinpoint() or even perjoinpointstaticpart() seems tough. In the former case would we create instance fields in the affected target for every advised joinpoint? If we didn't do that then we would be reduced to maintaining a map from joinpoint>aspect instance - and map lookups is the whole thing we are trying to avoid here. A perjoinpointstaticpart() seems more achievable because we already have a field in the affected target for every JoinPointStaticPart - but as mentioned earlier in this bug, they are shared - there is not one per aspect affecting the type. This means we couldn't just extend what we have to add an aspect instance to the existing JoinPointStaticPart field, because if there was more than one aspect, they'd clash. Maybe the use cases for large scale crosscutting (tracing) and small scale crosscutting (profiling) don't overlap and so I'm worrying too much about the problem of having one instance field in the target type per joinpoint per aspect instance, since in the scenarios where the instantiation model is useful there are not thousands of advised join points. How many places are you advising with your profiling aspect, for example? It does seem that a way to reduce the map size for these kinds of aspect is to also use the pertypewithin() instantiation model. Instead of a singleton aspect with a huge map of all thisJoinPointStaticPart instances to StopWatches, it would be one map per advised type that only included tJPSPs from that advised type.
I spoke with Ramnivas who agreed that the pertypewithin(*) lifecycle for an aspect would really help limit the size of the maps involved (and so speed up lookup time). However, I've also spent the day thinking about perjoinpoint and perjoinpointstaticpart. There might be something we should do for these instantiation models that would offer great performance - but the trade off would be larger class files, and possibly files that are tooo big for a highly pervasive aspect attempting to use them, so we'd have to police how many joinpoints are being advised within a particular type. However, adding a new instantiation model is not cheap because we need to do it properly and write an army of testcases. Whereas a very basic set/get userobject for thisjoinpointstaticpart looks like it would address every reasonable use case I have seen and is far less effort (just an API change rather than a language change). I might think about this small change for 1.6.4.
Hi Andy, thank you for putting this much effort into this, really appreciate it. In my use case the number of affected classes would not be more than a couple of hundreds. I do not need to use the pertypewithin instantiation model because i can work around it by injecting the map into the advised object and use it directly from there which avoids having a huge map in a singleton aspect. All i need is a single unique identifier(from a reasonable range) of joinpoints. With that identifier i can inject an array into the advised object and lookup my stopWatch from that based on this identifier.Doing array lookups instead of field lookups is not much of a big deal and seems to me the closest we can get. The fallback of this approach arises when we want to use more than one thing for profiling. In this case for every "profiler" we have to inject a field to the advised class even if that class does not use let's say accessCounter. What's your thought on this approach? Zoltan
> I do not need to use the pertypewithin instantiation model because i can work > around it by injecting the map into the advised object and use it directly from > there which avoids having a huge map in a singleton aspect. Yep, sounds good, that is just about the same thing as pertypewithin(*) would give you. (pertypewithin is really just implemented as holding the aspect instance as a new field in the target types). > All i need is a single unique identifier(from a reasonable range) of > joinpoints. With that identifier i can inject an array into the advised object > and lookup my stopWatch from that based on this identifier.Doing array lookups > instead of field lookups is not much of a big deal and seems to me the closest > we can get. > > The fallback of this approach arises when we want to use more than one thing > for profiling. In this case for every "profiler" we have to inject a field to > the advised class even if that class does not use let's say accessCounter. It sounds reasonable to me. As long as you don't have too many profilers I guess there won't be too many extra fields in the target objects. If I add just the: void JoinPoint.StaticPart.setUserObject(Object); Object JoinPoint.StaticPart.getUserObject(); do you think that would be entirely sufficient for you? I suppose you'd use it to hold the unique identifier generated from the thisJoinPointStaticPart info? And keep the range down so it can be the array index. That would be a minimal amount of work for me and achievable in a few days. If it was required on the individual JoinPoint level, that would be a much larger piece of work which I couldn't manage in 1.6.4 and it would be unlikely in 1.6.5/1.6.6. I think the initial thing I'm proposing here is how I'd like to proceed and then collect feedback on it and perhaps grow it into perjoinpointstaticpart/perjoinpoint aspect models sometime later.
Adding those API's to the StaticPart is problematic because of the thing mentioned in this issue: The StaticParts are shared among aspect's, if i consume that space for my lib i am closing everyone else out. I have checked the code in the generated classes and found that the static part fields are named as ajc$tjp_0,ajc$tjp_1 ... ajc$tjp_N. If those order numbers were to be exposed, i would be able to build my logic around that and get rid of the map lookup completely. What about adding the following API: int JoinPoint.StaticPart.getOrderNumber(); How feasible is this proposal? Taking this thought forward implementing the perjoinpoint aspect model would be much easier: 1) You can inject an array of aspects into the advised object. 2) At every joinpoint you can lookup the aspect instance from the array based on the order and execute the required function on that. What do you think?
> Adding those API's to the StaticPart is problematic because of the thing > mentioned in this issue: The StaticParts are shared among aspect's, if i > consume that space for my lib i am closing everyone else out. yep, you are closing everyone else if you use the slot, unless it is made into a Map, each aspect keying what it is using in there. And we'd be back to map lookups - and they'd be slow. I knew this was a limitation of that solution but if it addressed a common use case, it seemed reasonable. But it does move all the responsibility to the aspect writer. It sounds like you don't want this, so I'll shelve it again for now. Since I've been thinking more about perjoinpoint/perjoinpointstaticpart - they seem a better solution to this problem of state. > I have checked the code in the generated classes and found that the static > part fields are named as ajc$tjp_0,ajc$tjp_1 ... ajc$tjp_N. If those order > numbers were to be exposed, i would be able to build my logic around that > and get rid of the map lookup completely. > > What about adding the following API: > int JoinPoint.StaticPart.getOrderNumber(); > > How feasible is this proposal? Hmmmmm, relatively easy to implement but there are no guarantees that the numbers will remain the same across builds. If a single new joinpoint were to be advised and it resulted in a new field, it would be inserted at some point in the list, not at the end. I think getOrderNumber() suggests the ordering is more reliable than it really is. In your situation you possibly don't really care about the number changing across builds so long as it remains the same across multiple invocations. But really I'm not too keen on externalizing a value we can't give an accurate definition to, it feels a bit hacky. And for two thisJoinPointStaticParts in different files, it would return the same value, so it is not a unique number across the system - that may surprise some users, so maybe it would need qualification ... getIdWithinAffectedType(). I guess you are ok with that because of the injected map being type specific but that is also perhaps a factor against the general usefulness of a change like this. > Taking this thought forward implementing the perjoinpoint aspect model > would be much easier: > 1) You can inject an array of aspects into the advised object. > 2) At every joinpoint you can lookup the aspect instance from the array > based on the order and execute the required function on that. > > What do you think? I'm afraid for (2) - we don't have fields for joinpoints, they are conjured up on demand on the stack because every time they are encountered they are likely to be different. We only have fields for joinpointstaticparts since that information (line number/etc) is unchanging (you can see the type of the ajc$tjp objects is 'JoinPoint$StaticPart'. So going down this road the only quick-ish solution would be for a perjoinpointstaticpart instantiation model (since adding fields for all join points would be a very large piece of work). Now that may be a sensible stop-gap on the road to supporting perjoinpoint, but I can't fit it into 1.6.4. I do like your idea of the tjp static part number indexing the instance (since the number can then just be an 'internal' thing, which the user is never aware of, and it potentially changing across builds will then not matter). But then what would be the signature for the aspectOf() method for perjoinpointstaticpart/perjoinpoint instantiation model? It can't be aspectOf(orderNumber) because the number may change across compiles and the user cannot rely on it. It is these surrounding issues that need solving for an entirely new language feature like this. Would we even need an end user version of aspectOf() and hasAspect()? I've not thought about it enough, but we have them for every other kind of instantiation model so it feels odd if they were missing. this is some good discussion and is slowly teasing out some new language features and their design :)
More thinking... Given that a join point is an event that occurs during the execution of a program, they are all different (even though if you think quickly about it, some may appear to be the same). So a perjoinpoint instantiation model, in the purest sense, would run every piece of advice on a new aspect instance. That isn't so useful. So I think the best solution is perjoinpointstaticpart, but I still can't crack what aspectOf()/hasAspect() take as parameters. If I could crack that, we'd be in good shape. Oh and let me also reference another related bug, this one covers supporting a custom joinpoint factory so the user could specify the factory for joinpoint objects and do whatever they wish in their creation: bug 160296
I guess the signature for it is really aspectOf(JoinPoint.StaticPart) but of course I'd been getting ahead of myself because how would the user ever be able to determine the object to pass from some arbitrary context. This made me think of a simple (possibly generally useful) API to return the joinpoint static parts for a type JoinPoint.StaticPart[] joinpointsIn(Class) So if a user realllllllly realllly needed to call aspectOf, they had everything they need to do it. Of course internally the aspectj implementation would be highly optimized to get straight to the aspect in question. aspect CleverStuff perjoinpointstaticpart() { private Object importantThing; before(): execution(* Foo.*(..)) { if (importantThing==null) { importantThing = createImportantThing(); } // use importantthing } } then each time the joinpoint is reached we would use the same importantThing. Under the covers class Foo { CleverStuff[] ajc$instances; JoinPoint.StaticPart ajc$tjp_0; JoinPoint.StaticPart ajc$tjp_1; JoinPoint.StaticPart ajc$tjp_2; JoinPoint.StaticPart[] ajc$allJPs; static { ajc$instances = new CleverStuff[3]; ajc$tjp_0= make the first joinpoint ajc$tjp_1= make the first joinpoint ajc$tjp_2= make the first joinpoint } CleverStuff optimizedAspectOf(int i) { if (ajc$instances[i]==null) { ajc$instances[i]= new CleverStuff(); } return ajc$instances[i]; } JoinPoint.StaticPart[] joinpointsIn() { // created on demand for the 3 join points } // Advised method public void m() { optimizedAspectOf(0).ajc$before$Advice(); } } class CleverStuff { // similar for hasAspect public CleverStuff aspectOf(JoinPoint.StaticPart jpsp) { return Foo.optimizedAspectOf(jpsp.internalId); } public void ajc$before$advice() { } } the use of a more optimal aspectOf in the target type would be similar to what happens for pertypewithin, and there are no map lookups.
> I think getOrderNumber() suggests the ordering is more reliable than it really is I was always bad at naming :-) getIdWithinAffectedType() sounds much better >Given that a join point is an event that occurs during the execution of a >program, they are all different (even though if you think quickly about it, >some may appear to be the same). So a perjoinpoint instantiation model, in the >purest sense, would run every piece of advice on a new aspect instance. That >isn't so useful. I totally agree with that, creating an aspect at each execution of a joinpoint is useless and can be already achieved with the existing features(just create an adivse for that joinpoint and create the state which would be stored in the aspect by yourself). In my dictionary i have the following definition for the perjoinpoint instantiation model: It will instantiate 1 aspect for each joinpoint which instance's lifecycle spans among all joinpoint execution at that given point in code. One thing which needs to be mentioned here is having an Aspect instance per joinpoint in the advised class as a static member will beat the purpose of this instantiation model, because at every execution of a given advise i have to look up the state of the current object at the current joinpoint, which would involve the map lookup again. Let me share my original thought: aspect CleverStuff perjoinpoint() { private Object importantThing; //Here the jpsp can be optional, but it could be used to load the state of this aspect instance.(Also adding the this object to the constructor would be nice) public CleverStuff(JoinPoint.StaticPart jpsp){ importantThing = createImportantThingBasedOnTheStaticPart(jpsp); } before(): execution(* Foo.*(..)) { // use importantthing } } would transform to the following class: class CleverStuff{ public interface Advised{ CleverStuff[] getAspects(); } public static boolean hasAspect(Object object){ return object instanceof Advised; } public static CleverStuff[] aspectOf(Object object){ if(!hasAspect(object)){ throw new AspectNotBoundException(); } return ((Advised)object).getAspects(); } private Object importantThing; public CleverStuff(JoinPoint.StaticPart jpsp){ importantThing = createImportantThingBasedOnTheStaticPart(jpsp); } public void ajc$before$advice() { } } and then under the cover: class Foo implements CleverStuff.Advised{ CleverStuff[] ajc$instances; static JoinPoint.StaticPart ajc$tjp_0; static JoinPoint.StaticPart ajc$tjp_1; static JoinPoint.StaticPart ajc$tjp_2; static{ ajc$tjp_0= make the first joinpoint ajc$tjp_1= make the first joinpoint ajc$tjp_2= make the first joinpoint } { ajc$instances = new CleverStuff[3]; ajc$instances[0] = new CleverStuff(ajc$tjp_0); ajc$instances[1] = new CleverStuff(ajc$tjp_1); ajc$instances[2] = new CleverStuff(ajc$tjp_2); } public CleverStuff[] getAspects(){ return ajc$instances; } // Advised method public void m() { ajc$instances[0].ajc$before$Advice(); } } With this approach we can guarantee that at every joinPoint a piece of state can be attached to the joinPoint, which state is not shared between different objects. This approach has some fallbacks: 1) Supporting the pre-initialization and initialization pointcuts would require adding a static aspect instance to the field 2) The same for pointcuts matching on static functions, they would also require a static aspect instance field, to guarantee the same instance of aspect will be used at every joinpoint execution. Given that this kind of instantiation model at it's current state can be almost achieved with the existing language features i do not think it worth having the burden of implementing it at the language level.(If the internal id of the static part is reachable than i can store the state what i would store in the aspect instance in the advised class and look it up from there + having a nice javadoc for getIdWithinAffectedType()) But if we want to make the above solution perfect then we have to inject extra static fields. But if we have to do that then we can choose a slightly different approach: Inject an aspect field to the advised class for each joinpoint, which can be static if the underlying pointcut requires it. The aspect implementation would not change just the woven class: class Foo implements CleverStuff.Advised{ //This join point is a static initialization joinpoint and it requires to run before any other initialization steps is made. static final JoinPoint.StaticPart ajc$tjp_0; static final CleverStuff ajc$cleverStuffAspect_0; //This join point matches on a static function/field or for pre-initalization /initialization joinpoints static final JoinPoint.StaticPart ajc$tjp_1; static final CleverStuff ajc$cleverStuffAspect_1; //No comes the joinpoints for instance members static JoinPoint.StaticPart ajc$tjp_2; CleverStuff ajc$cleverStuffAspect_2 static JoinPoint.StaticPart ajc$tjp_3; CleverStuff ajc$cleverStuffAspect_3 CleverStuff[] ajc$instances; static{ ajc$tjp_0= make the first joinpoint ajc$cleverStuffAspect_0 = new CleverStuff(ajc$tjp_0); ajc$tjp_1= make the first joinpoint ajc$cleverStuffAspect_1 = new CleverStuff(ajc$tjp_1); ajc$tjp_2= make the first joinpoint ajc$tjp_3= make the first joinpoint } { ajc$instances = new CleverStuff[4]; ajc$instances[0] = ajc$cleverStuffAspect_0; ajc$instances[1] = ajc$cleverStuffAspect_1 ajc$instances[2] = ajc$cleverStuffAspect_2 = new CleverStuff(ajc$tjp_2); ajc$instances[3] = ajc$cleverStuffAspect_3 = new CleverStuff(ajc$tjp_3); } public CleverStuff[] getAspects(){ return ajc$instances; } // Advised method public void m() { ajc$cleverStuffAspect_2.ajc$before$Advice(); } } This indeed increase the size of the woven class, but the use cases which would use it wouldn't mind it.(If i want to write a trace aspect than i wouldn't use this instantiation model ). And also i can imaging it would take much more work than implementing the getIdWithinAffectedType() method. >Oh and let me also reference another related bug, this one covers supporting a >custom joinpoint factory so the user could specify the factory for joinpoint >objects and do whatever they wish in their creation: bug 160296 If we were implementing the perjoinpoint instantiation model as described above, than the need for joinpoint factory would disappear: You would get an aspect instance/joinpoint/object and you can store any kind of state/logic in there instead of hacking that state/logic into the joinpoint itself(which is created on the fly several times) We are getting there :-)
And one more thing: I am really bad at naming, feel free to change any names in the code i have written. :-)
What do you think about this approach? Is it feasible?
I have a full design for perjoinpoint which seems to work - I have written the generated code as 'real code' to test it works as expected (it is rather similar to the pertypewithin approach, in terms of the architecture of what methods go where). It is a lot of effort though and also hinges on joinpoint static parts having an ID. So, as a precursor to that which will address this enhancement request in the short term, I'm on the verge of adding getId() to joinpoints (I prefered a very simple name in the end), a simple id you can use it as an array index. Javadoc for getId() is currently: /** * Return an id number for the join point static part. All join points * advised in a target type are assigned an id number, starting from 0. * This number is guaranteed to remain constant across repeated executions * of a program but may change if the code is recompiled. In each affected * type the id numbers start from 0 so two join points advised in different types * may have the same number. The benefit in having an id is for rapid access * to per joinpoint state through an array lookup rather than using the * joinpoint object itself as a key in a map lookup. however, because * the numbers are not unique across types, any maintained state must be * maintained per type (either by using an ITD or the pertypewithin * instantiation model). * * @return the id of this joinpoint */ The last stumbling block is me deciding if this is ok from a language point of view. --- For reference, the working perjoinpoint code is here. import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import org.aspectj.lang.JoinPoint; import org.aspectj.lang.NoAspectBoundException; import org.aspectj.lang.JoinPoint.StaticPart; import org.aspectj.runtime.reflect.Factory; public class C { private static AspectX[] ajc$AspectX$instances; private static JoinPoint.StaticPart[] ajc$AspectX$joinpoints; // created on demand as probably not used very often private static final int ajc$AspectX$maxJps; private static final org.aspectj.lang.JoinPoint.StaticPart ajc$tjp_0; private static final org.aspectj.lang.JoinPoint.StaticPart ajc$tjp_1; static AspectX ajc$AspectX$localAspectOf(int id) { if (id >= ajc$AspectX$maxJps) { return null; } return ajc$AspectX$instances[id]; } static JoinPoint.StaticPart[] ajc$AspectX$getJoinpoints() { if (ajc$AspectX$joinpoints == null) { ajc$AspectX$joinpoints = new JoinPoint.StaticPart[ajc$AspectX$maxJps]; ajc$AspectX$joinpoints[0] = ajc$tjp_0; ajc$AspectX$joinpoints[1] = ajc$tjp_1; } return ajc$AspectX$joinpoints; } static { Factory f = new Factory("C.java", C.class); ajc$AspectX$maxJps = 2; ajc$tjp_0 = f.makeSJP(0, "method-execution", f.makeMethodSig("1-m-C----void-"), 33); // 33 is line number ajc$tjp_1 = f.makeSJP(1, "method-execution", f.makeMethodSig("1-n-C----void-"), 38); ajc$AspectX$instances = new AspectX[ajc$AspectX$maxJps]; ajc$AspectX$instances[0] = AspectX.ajc$createAspectInstance(ajc$tjp_0); ajc$AspectX$instances[1] = AspectX.ajc$createAspectInstance(ajc$tjp_1); } public void m() { // jpsp is only passed because it is used in the advice, it isn't necessary for perjp to work ajc$AspectX$instances[0].ajc$before$AspectX$1$3444dde4(ajc$tjp_0); System.out.println("m running"); } public void n() { // advice calls are as fast as can be, a swift array access and call ajc$AspectX$instances[1].ajc$before$AspectX$1$3444dde4(ajc$tjp_1); System.out.println("n running"); } public static void main(String[] args) { new C().m(); new C().n(); new C().m(); new C().n(); new C().m(); new C().n(); JoinPoint.StaticPart[] jpsps = AspectX.joinpointsIn(C.class); System.out.println("number of joinpointstaticparts in class C because of AspectX " + jpsps.length); JoinPoint.StaticPart jp1 = AspectX.joinpointsIn(C.class)[0]; JoinPoint.StaticPart jp2 = AspectX.joinpointsIn(C.class)[1]; System.out.println("Aspect instance for " + jp1 + " is " + AspectX.aspectOf(jp1)); System.out.println("Aspect instance for " + jp2 + " is " + AspectX.aspectOf(jp2)); } } class AspectX { int i = 0; private StaticPart ajc$jpsp; static AspectX aspectOf(JoinPoint.StaticPart jpsp) { AspectX instance = ajc$getInstance(jpsp); if (instance == null) { throw new NoAspectBoundException(); } return instance; } static boolean hasAspect(JoinPoint.StaticPart jpsp) { AspectX instance = ajc$getInstance(jpsp); return instance != null; } public static AspectX ajc$createAspectInstance(StaticPart jpsp) { // depending on whether the user specified a ctor that takes a jpsp, call: AspectX ax = new AspectX(); // or // AspectX ax = new AspectX(jpsp); ax.ajc$jpsp = jpsp; return ax; } public JoinPoint.StaticPart getJoinpoint() { return ajc$jpsp; } public static JoinPoint.StaticPart[] joinpointsIn(Class c) { try { Method m = c.getDeclaredMethod("ajc$AspectX$getJoinpoints"); return (StaticPart[]) m.invoke(null); } catch (IllegalArgumentException e) { return null; } catch (SecurityException e) { return null; } catch (IllegalAccessException e) { return null; } catch (NoSuchMethodException e) { return null; } catch (InvocationTargetException e) { return null; } } private static AspectX ajc$getInstance(JoinPoint.StaticPart jpsp) { try { Method m = jpsp.getSignature().getDeclaringType().getDeclaredMethod("ajc$AspectX$localAspectOf", Integer.TYPE); return (AspectX) m.invoke(null, jpsp.getId()); } catch (SecurityException e) { return null; } catch (NoSuchMethodException e) { return null; } catch (IllegalArgumentException e) { return null; } catch (IllegalAccessException e) { return null; } catch (InvocationTargetException e) { return null; } } public void ajc$before$AspectX$1$3444dde4(JoinPoint.StaticPart jpsp) { System.out.println("advice running at " + jpsp + " loc=" + jpsp.getSourceLocation()); System.out.println("i=" + i); i++; } }
There will not be any API alongside it to ask 'getIdMax()' or anything - is that a problem for you?
(In reply to comment #26) > There will not be any API alongside it to ask 'getIdMax()' or anything - is > that a problem for you? No, i can handle the size of the array without the need of getIdMax(). Checking through your perjoinpoint implementation it seems to me it is more like a per_class_joinpoint instantiation model as defined by JBOSS Aop, while my proposal seems to be nearer to the per joinpoint model: Copied from JBOSS AOP doc(http://www.jboss.org/jbossaop/docs/2.0.0.GA/docs/aspect-framework/reference/en/html/xml.html#xml-aspect2): "PER_CLASS_JOINPOINT: An instance of an aspect will be created per advised joinpoint. The aspect instance is shared between all instances of the class (for that joinpoint). PER_JOINPOINT: An instance of an aspect will be created per joinpoint advised. If the joinpoint is a static member (constructor, static field/method), then there will be one instance of the aspect created per class, per joinpoint. If the joinpoint is a regular non-static member, than an instance of the aspect will be created per object instance, per joinpoint." Based on these is there any chance to add an aspect/joinpoint/object instantiation model in the (near) future? If yes, which could be the target release? The getId() API seems to be the golden path for now. Do you have an ETA for it? Thank you again for your continuous effort on this, it was a pleasure for me brainstorming on this issue.
Thanks for digging that out from the Jboss doc. Yes, this is what jboss call per_class_joinpoint - which is what I've been calling all along 'perjoinpoint staticpart' or 'perjoinpoint shadow' - it is per thing that is constant across all instances of a join point. It seems unfortunately have been slightly at cross purposes in the discussion because of the difference in terminology. This model (perjoinpoint shadow) is more feasible (as in much easier to implement in the short term) than perjoinpointperinstance because of how much infrastructure is already around in the advised classes. perjoinpointperinstance is something I've not put any thought into, and I'd need some compelling use cases to explore it further (I'll look through your handwritten generated code again when i get time). I wonder how far it can be achieved already with a combination of pertypewithin/getId for the static components and perthis/getid for the non-static components. > Based on these is there any chance to add an aspect/joinpoint/object > instantiation model in the (near) future? If yes, which could be the target > release? Not really on my radar right now without some compelling use cases, but not in 1.6.4. > The getId() API seems to be the golden path for now. Do you have an ETA > for it? might do it today. > Thank you again for your continuous effort on this, it was a pleasure for me > brainstorming on this issue. yes, enjoyed it - shame we were a little at cross purposes due to a difference in terminology though.
Great design discussion. I wanted to check if the current plan would provide support for instance field level manipulation. In my comment #10 below (https://bugs.eclipse.org/bugs/show_bug.cgi?id=89009#c10) I refer to Dash Obtain that lazy-loads fields in instances. Would the proposed near-term solution support the following: 1) Given: public class Foo { @Obtain DataSource dataSource; @Obtain ISomeService someService; } 2) Using ITD: Introduce a per-instance AtomicMarkableReference[] array into any class that hasfield(@Obtain *). 3) Would the following pointcut get a unique id _per field_, or a single unique id? pointcut obtain_get(): get(@Obtain !static * *); 4) Lookup the correct AtomicMarkableReference by array index (and appropriate synchronization), then do actual stuff. Thanks, John Heintz
I have spun off two enhancement requests for: perjoinpointstaticpart/perjoinpointshadow/perclassjoinpoint - bug 269847 perjoinpointperinstance/'perjoinpoint' - bug 269850 The former is more well formed in my mind, whilst the latter needs some good use cases. In both cases they are kind of achievable using existing constructs - and achievable in a performant way when getId() is added. Now to think about John's situation. With getId() there is a unique id for each join point static part - the problem is that accessing the same field from different places would be considered different join points (as John imagines in his point (3)) These are four different join point (shadows): ... someinstance.accessFieldA someinstance.accessFieldB someinstance.accessFieldA someinstance.accessFieldB ... so each has its own ID, even though only two fields are accessed. I don't think perthis or pertarget will help. Still thinking... (I don't imagine either of the two new enhancement requests will help either). I'm currently wondering if there is a way to write an aspect such that it is costly to initially sort out the references but then fast afterwards due to array lookups. Here is the brain bender so far: aspect BrainBender { interface Marker {} declare parents: hasfield(@Obtain * *) implements Marker; // Array used for fast lookup, lazily initialized. Not one entry per field - // there are possibly multiple entries for the same field, but under // different ids (they will share the same AtomicMarkableReference). AtomicMarkableReference[] Marker.amr = new AtomicMarkableReference[100]; // Hideous map used when we haven't initialized the array for a // particular join point id Map<Marker,Map<String/*FieldName*/,AtomicMarkableReference>> map = new HashMap<Marker,Map<String,AtomicMarkableReference>>(); before(Marker m): get(@Obtain * *) && target(m) { // if this succeeds - zoom AtomicMarkableReference amr = m.amr[thisJoinPointStaticPart.getId()]; // otherwise very costly if (amr==null) { int idx = thisJoinPointStaticPart.getId(); // Find it out the expensive way String fieldName = thisJoinPointStaticPart.getSignature().getName(); System.out.println("Field access of "+fieldName+" on instance "+m); Map<String,AtomicMarkableReference> instanceMap = map.get(m); if (instanceMap == null) { map.put(m,instanceMap=new HashMap<String,AtomicMarkableReference>()); } else { amr = instanceMap.get(fieldName); } if (amr==null) { amr = new AtomicMarkableReference(); instanceMap.put(thisJoinPointStaticPart.getSignature().getName(),amr); } m.amr[idx] = amr; // will be fast next time } System.out.println("Accessing a field, joinpoint id = " +thisJoinPointStaticPart.getId()+" amr="+amr); } } Less than perfect (does it even meet your needs?), but once the arrays are setup it is as fast as can be. It means multiple references will be kept to an AtomicMarkableReference in the array, one reference for each joinpoint index that is accessing the same field. The map I use in the slow case to initialized the array sounds a bit like your (from comment 10) 'The aspect I've already implemented is constructing a ConcurrentHashMap to map from field to AtomicMarkableReference' A possible enhancement of perfield() will need more thought and its own enhancement request - what it means currently isn't completely clear in my mind.
Andy, that latest BrainBender proposal should work. The code you wrote would need a little synchronized voodoo, but certainly doable. I like the _clever_ trick of storing the "multiple references will be kept to an AtomicMarkableReference in the array", that would optimize the normal lookup path and only cause pain during first reference. I even have a brain-dead performance test in Dash Obtain to prove out the implications of different strategies, http://code.google.com/p/dash-framework/source/browse/trunk/dash-obtain/tests/dash/obtain/performance/SequentialTiming.java Here's a bit of that code: // throw away first results... getLoopTiming(createDirectConsumers(1), 1); getLoopTiming(createAtomicConsumers(1), 1); getLoopTiming(createIndexedAtomicConsumers(1), 1); getLoopTiming(createFieldObtainedConsumers(1), 1); The last one is the AspectJ code, it runs about an order of magnitude slower than the first three. With your suggested code the performance should be about the same. The startup time would be measurable, but heck I'm getting objects from Spring and properties file! Who cares! :) Cheers, John
The change is in. The dev builds later today will include thisJoinPointStaticPart.getId(). Please try it out. Going to have 50 zillion bugs reported against 1.6.4 now because people are using getId() but not running with an up to date aspectjrt.jar
It worked, and it's quite a bit faster. (Given a trivial looping test...) The new timings are: > ant sequential ... sequential: [java] Do timings for 1000 consumers over 5000 iterations. [java] Initial creation timings: [java] Control is Direct Consumer at 51 [java] Atomic Consumer is 1 [69] [java] Indexed Consumer is 1 [79] [java] Field Consumer is 7 [376] [java] [java] Looped access timings: [java] Control is Direct Consumer at 64 [java] Atomic Consumer is 1 [73] [java] Indexed Consumer is 1 [94] [java] Field Consumer is 2 [154] [java] What that means: * "Field Consulmer is 2": Looped access timings are only 2X the cost of a pure Java solution with direct field access. That sounds pretty darn good for AOP intercepted access. * Initial processing time was 7X the other strategies. This number isn't really a big deal, as the time measured here will be swamped by actually loading Spring objects for example. Here is the timing run from the old code that used a giant shared ConcurrentHashMap: sequential: [java] Do timings for 1000 consumers over 5000 iterations. [java] Initial creation timings: [java] Control is Direct Consumer at 173 [java] Atomic Consumer is 1 [193] [java] Indexed Consumer is 1 [185] [java] Field Consumer is 9 [1668] [java] [java] Looped access timings: [java] Control is Direct Consumer at 167 [java] Atomic Consumer is 1 [177] [java] Indexed Consumer is 1 [186] [java] Field Consumer is 9 [1559] [java] This was essentially an order of magnitude slower for the common case of lookups (not counting intialization) and seemed to costly to me. Thanks all for driving this progress and a workable solution.
unsetting the target field which is currently set for something already released
