Community
Participate
Working Groups
This enhancement is intended to make AspectJ expose join points for synchronized code and define the pointcuts that will match on these join points. Jonas has a nice blog article on the issues: http://jonasboner.com/2005/07/18/semantics-for-a-synchronized-block-join-point/ Current thoughts for an implementation are: - transform synchronized methods to a method containing a synchronized block - pointcuts for monitorentry and monitorexit, exposing what-is-being-sync'd on as context Current worries, please append more as you think of them: - transforming methods and 'removing' the synchronized flag alters the declared signature... - are their subtle JVM differences in how a JVM treats synchronized methods and methods containing synchronized blocks - I cant find any differences, but you never know. - can you use 'proceed' in an around advice to lock on a different object - is this good/bad...
Another concern is to ensure that creating the necessary join points for synchronized methods does not affect matching by other pointcuts either during the same or subsequent weaving steps i.e. the following advice will be invoked from _within_ the expanded sychronization block not at the start and end of the method: after () : execution(synchronized * *(..)) {}
Created attachment 37732 [details] Shock horror, some design notes Pulling together some thoughts on a possible design and the problems it raises...
Slide comments: 5. Additional potential use cases (that need consistent before lock() advice): - Deadlock detection/avoidance - Deadlock diagnosis 10. Prefer lock() & unlock() 12. As far as I can tell synchronized methods and blocks are semantically identical and must be matched as such if the identified use cases are to work. This means auto transformation is required perhaps only with a privileged aspects because method signatures can be modified). However the weaving of one aspect should never (correct me if I am wrong) affect whether pointcuts in another match, regardless of when the weaving takes place i.e. at the same time or separately. 14. New slide for advice: before/after/after throwing
Design discussion meeting minutes ;) Friday 7th April 05: (14:40:57) Adrian Colyer: 123759 yes? (14:41:46) Andy Clement: yes 123759 (14:42:49) Adrian Colyer: oooh. funky template (14:45:30) Adrian Colyer: slide 10.... (14:45:50) Adrian Colyer: I would have expected the lock object to be bound in args (14:46:00) Adrian Colyer: you have synchronized(obj) (14:46:07) Adrian Colyer: obj is like the argument to me (14:46:23) Adrian Colyer: and then this and target would both be the same as for an execution jp (14:46:23) Andy Clement: yes, I think you are right, that's a mistake (14:46:46) Andy Clement: i think i was worrying more about the other problems when I was writing it. (14:47:07) Adrian Colyer: on slides 8 and 9... where you say 2 exit join points. at runtime only one would ever occur right? (14:47:17) Adrian Colyer: it 2 *shadows* that have to be woven? (14:47:30) Andy Clement: yes, 2 exit join points - i think only one occurs (14:47:43) Andy Clement: 2 shadows to weave (14:48:10) Andy Clement: that was my biggest worry I think, seeing 'two matches' (not in the gutter but in the showweaveinfo) would be confusing (14:48:57) Adrian Colyer: for after returning we must have a potentially similar situation? (14:49:00) Andy Clement: i need to double check ... but if it was a finally block rather than an exception block, they might both execute. (14:49:29) Adrian Colyer: have a mismatched number of lock and unlock join points would be very confusing (14:49:33) Adrian Colyer: if it happens at runtime (14:49:47) Andy Clement: the JVM may consider it fine to unlock something already unlocked. (14:50:01) Andy Clement: i was surprised to see two monitorexits. (14:50:29) Adrian Colyer: I'm very nervous about transformation (14:50:39) Adrian Colyer: I would leave that until a later iteration (14:51:18) Adrian Colyer: even though it's clearly desirable from a language perspective (14:51:31) Andy Clement: i am nervous about transformation too (14:51:59) Adrian Colyer: aha, you have that phasing anyway (14:52:07) Andy Clement: oh, i thought you'd seen that ;) (14:52:21) Adrian Colyer: phase 1 looks fine as a -X option (14:52:42) Adrian Colyer: phase 2 needs some hard thought, and probably a 1.6 item because of the change in semantics (14:53:55) Adrian Colyer: moving advice from inside to outside of a lock is very serious (14:54:11) Andy Clement: i agree (14:55:10) Adrian Colyer: I think it is an aj-transformed method, then we still have to put execution advice inside the aj -generated sync block (14:55:39) Adrian Colyer: lock / unlock would be different join point kinds (14:55:43) Andy Clement: not impossible, but not trivial (14:55:46) Adrian Colyer: so you can never have execution && lock (14:56:02) Adrian Colyer: so therefore just lock and unlock would have the outside / inside semantics (14:56:30) Adrian Colyer: I really don't think we can do it unless execution semantics are preserved (14:56:38) Andy Clement: i think that is a reasonable limitation isnt it? monitor<thing> is a kinded PCD, so cant be used with the other kinded pointcuts. (14:56:50) Adrian Colyer: It's necessary (14:57:04) Adrian Colyer: you can do within etc. (14:57:07) Andy Clement: yes (14:57:13) Adrian Colyer: but not any other kinded jp (14:57:43) Adrian Colyer: then we can define the semantics of lock and unlock jps precisely (for both blocks and sync methods) (14:58:05) Adrian Colyer: and keep the semantics of execution as they are now (14:58:22) Adrian Colyer: more work on the implementation, but the only way that transformation can be acceptable i fear (14:58:37) Andy Clement: execution(synchronized * *(..)) would match these transformed things then? (14:58:47) Adrian Colyer: yes (14:58:50) Adrian Colyer: it would have to (14:59:13) Andy Clement: it'll need more than a few testcases... (14:59:22) Adrian Colyer: the synchronized bit is held as a modifier, and we already have modifier massaging code for some jps don't we? (14:59:30) Adrian Colyer: itd visibility and stuff (14:59:38) Andy Clement: yes kind of. (15:00:27) Adrian Colyer: it's a lot of effort for the "debugging-my-locking" use case (15:00:32) Adrian Colyer: is there another good one? (15:01:16) Andy Clement: another use case? monitoring-my-locking? ;) (15:01:34) Adrian Colyer: of course, silly m (15:01:35) Adrian Colyer: e (15:01:42) Adrian Colyer: we could trace-my-locking too (15:02:47) Adrian Colyer: after() returning : unlock(xxx) would have to be immediately after the monitorexit instruction (15:02:52) Adrian Colyer: why is that hard? (15:03:00) Andy Clement: which monitorexit instruction? (15:03:13) Adrian Colyer: both of them, if they both execute (15:03:33) Andy Clement: and so you get two weave info messages when all you had in the original source was one synchronized block. (15:04:35) Andy Clement: after() returning : execution() knows that all RETURNS from the method mean the same thing => one showweaveinfo, advice stuffed in multiple places (15:04:36) Adrian Colyer: If we don't do that, we have to create an abstraction over the byte code, and weave against that abstraction, and I think that will get hard very quickly in non-trivial cases (15:04:55) Andy Clement: i agree, i dont want to get more complicated than necessary. (15:05:06) Adrian Colyer: so your only issue is actually managing the messages we put out??? (15:05:15) Adrian Colyer: surely that can be done! (15:06:21) Andy Clement: As we cannot tell the monitorexits are from the same block - without analysis - we will think they are different and advise both. now in the gutter view of course its the same 'line' so you don't see multiple gutters, but you will see multiple messages. (15:06:49) Adrian Colyer: but we see multiple returns and advise both, but only give one message (15:07:07) Andy Clement: yes, because RETURNS are all the same from a method, they mean leave the method. (15:07:27) Andy Clement: i dont know if you have two monitors or one monitor in the synchronized case. (15:07:51) Andy Clement: so we can advise all RETURNS and just put out a message/gutter saying - yes done that. (15:08:03) Adrian Colyer: if the block can throw exceptions that can escape the block, I bet you get multiple (15:08:23) Adrian Colyer: or maybe it's arranged like finally (15:08:36) Adrian Colyer: but then we're depending on a particular compilers compilation strategy (15:08:41) Adrian Colyer: which isn't right (15:08:49) Adrian Colyer: so we have to allow for an arbitrary number (15:08:56) Andy Clement: i will dig a bit more on the spec to see if its written down. (15:09:15) Andy Clement: but i hope you can see why its causing me concerns :( (15:09:35) Adrian Colyer: it will certainly confuse a lot of users (15:10:09) Andy Clement: <dirty hack> if both monitorexits have the same line number entry then they are for the same block</dirtyhack> ;) (15:10:44) Adrian Colyer: it's a bit analogous to building up Strings with + and then being surprised that there are join points for calls to StringBuffer / Builder (15:10:58) Adrian Colyer: the abstraction in the source code doesn't map quite so neatly to the bytecode in both cases (15:11:14) Adrian Colyer: but this one will happen more often and be more surprising I think (15:11:35) Adrian Colyer: <dirty-hack/> sounds like it would tackle a very large % the real-world cases (15:11:42) Andy Clement: well... i have to dash, thanks so much for going through the charts ... keep it rattling round your brains for a few days and let me know if you think of something elegant to solve it. (15:11:48) Andy Clement: i will rattle it round mine too. (15:12:00) Andy Clement: go walk the dog or something ;)
It seems to me that the new lock() and unlock() pointcuts are a hybrid of call(), handler() and something else: - call(): this() is bound to the currently executing object in a non-static method, target() is unbound (like a static method call) and args() is bound to the lock. You should also be able to use within() and withincode(). - handler(): the pointcut should take the form “lock/unlock(TypePat)” e.g. “lock(HelloWorld+)” so that specific types or type hierarchies can be targeted, especially with synchronized methods as opposed to blocks, without expensive residue. - The declaring type for the join point is tricky and perhaps should be java.lang.Object I very much doubt that monitorexit is called twice (see below for my analysis of the byte-code) because locks can be nested and a count is kept by the JVM. While monitorenter/exit be called separately in native code (http://java.sun.com/j2se/1.5.0/docs/guide/jni/spec/functions.html#wp5256) I strongly suspect that a strict FILO model is required in byte-code, something that is enforced by the synchronized() construct and if violated could cause a VerifyError (which we could check). Using a simple HelloWorld example I got close to the bytecode generated for a synchronized block. It seems to take the form of a try/catch with the caught exception rethrown in a try/finally. public class HelloWorld { public void println () throws Throwable { // public void println () { lock(this); try { System.out.println("Hello World!"); if (Math.random() < 1) throw new RuntimeException(); unlock(this); } catch (Throwable th) { try { unlock(this); } finally { throw th; } } } private static void lock (Object lock) { } private static void unlock (Object lock) { } public static void main(String[] args) throws Throwable { new HelloWorld().println(); } private Object that = new String("that"); public void printlnMultiSynchronizedBlock () { synchronized(this) { synchronized (that) { synchronized (this) { System.out.println("Hello World!"); } } } } } As you can see the real fun comes with trying to match monitorenter/exit for nested sycnchronized() blocks. The Aspect shows how lock/unlock() is similar to call(): public aspect Aspect { private pointcut lock (Object lock) : // lock(Object) call(private void lock(Object)) && args(lock); before (Object lock) : lock(lock) { System.out.println("Aspect.beforeLock() this=" + thisJoinPoint.getThis() + ", lock=" + lock); } private pointcut unlock (Object lock) : // unlock(Object) call(private void unlock(Object)) && args(lock); after (Object lock) : unlock(lock) { System.out.println("Aspect.afterUnlock() this=" + thisJoinPoint.getThis() + ", lock=" + lock); } before (Object ex) : handler(Exception+) && args(ex) { System.out.println("Aspect.beforeHandler() this=" + thisJoinPoint.getThis() + ", target=" + thisJoinPoint.getTarget() + ", ex=" + ex + ", sig=" + thisJoinPoint.getSignature() + ", type=" + thisJoinPoint.getSignature().getDeclaringType()); } } When run it ensures unlock() is called and the caught exception propagated: Aspect.beforeLock() this=HelloWorld@276af2, lock=HelloWorld@276af2 Hello World! Aspect.afterUnlock() this=HelloWorld@276af2, lock=HelloWorld@276af2 Exception in thread "main" java.lang.RuntimeException at HelloWorld.println(HelloWorld.java:19) at HelloWorld.main(HelloWorld.java:39) The byte-code (without aspect) for println() looks like this: public void println() throws java.lang.Throwable; Code: 0: aload_0 1: invokestatic #46; //Method lock:(Ljava/lang/Object;)V 4: getstatic #31; //Field java/lang/System.out:Ljava/io/PrintStream; 7: ldc #33; //String Hello World! 9: invokevirtual #38; //Method java/io/PrintStream.println:(Ljava/lang/String;)V 12: invokestatic #52; //Method java/lang/Math.random:()D 15: dconst_1 16: dcmpg 17: ifge 28 20: new #54; //class java/lang/RuntimeException 23: dup 24: invokespecial #55; //Method java/lang/RuntimeException."<init>":()V 27: athrow 28: aload_0 29: invokestatic #58; //Method unlock:(Ljava/lang/Object;)V 32: goto 46 35: astore_1 36: aload_0 37: invokestatic #58; //Method unlock:(Ljava/lang/Object;)V 40: goto 44 43: pop 44: aload_1 45: athrow 46: return Exception table: from to target type 4 35 35 Class java/lang/Throwable 36 43 43 any
> - call(): this() is bound to the currently executing object in a > non-static method, target() is unbound (like a static method call) and args() > is bound to the lock. You should also be able to use within() and withincode(). Yes. > - handler(): the pointcut should take the form “lock/unlock(TypePat)” > e.g. “lock(HelloWorld+)” so that specific types or type hierarchies can be > targeted, especially with synchronized methods as opposed to blocks, without > expensive residue. It isn't quite the same so i can't do that - we know something about the exception being caught at a handler join point, so can support handler(TypePat) and reduce the residue. For a monitorentry/exit, we know *nothing* about the argument to the call apart from that it is an 'Object'. The argument is on the stack. So, you will have to rely on args() to pick it out (and the residue that comes with it). So the forms will have to be 'lock()' and 'unlock()'. > - The declaring type for the join point is tricky and perhaps should be java.lang.Object yes, i think Object is probably safest Here is the progress so far this morning: ====8<======== // Exploring synchronization aspect LockMonitor { long locktimer = 0; int iterations = 0; Object currentObject = null; long activeTimer; before(Useful2 o ): lock() && args(o) { activeTimer = System.currentTimeMillis(); currentObject = o; } after(Useful2 o ): unlock() && args(o) { if (o!=currentObject) { throw new RuntimeException("Unlocking on incorrect thing?!?"); } if (activeTimer!=0) { locktimer+=(System.currentTimeMillis()-activeTimer); iterations++; activeTimer=0; } } after() returning: execution(* main(..)) { System.err.println("Average time spent with lock over "+iterations+" iterations is "+ (((double)locktimer)/ ((double)iterations))+"ms"); } } public class Useful2 { public static void main(String[] args) { Useful2 u = new Useful2(); for (int i = 0; i < 20; i++) { u.methodWithSynchronizedBlock(); } } public void methodWithSynchronizedBlock() { synchronized (this) { for (int ii=0;ii<1000000;ii++); } } } =====8<========== Compiling: weaveinfo Join point 'method-execution(void Useful2.main(java.lang.String[]))' in Type 'Useful2' (Useful2.java:33) advised by afterReturning advice from 'LockMonitor' (Useful2.java:25) weaveinfo Join point 'monitor-enter(void java.lang.Object.monitorenter(java.lang.Object))' in Type 'Useful2' (Useful2.java:42) advised by before advice from 'LockMonitor' (Useful2.java:9) [with runtime test] weaveinfo Join point 'monitor-exit(void java.lang.Object.monitorexit(java.lang.Object))' in Type 'Useful2' (Useful2.java:42) advised by after advice from 'LockMonitor' (Useful2.java:14) [with runtime test] weaveinfo Join point 'monitor-exit(void java.lang.Object.monitorexit(java.lang.Object))' in Type 'Useful2' (Useful2.java:42) advised by after advice from 'LockMonitor' (Useful2.java:14) [with runtime test] The two weave info messages for exit as expected. Confusing because they both give the same line number. I guess my dirtyhack to suppress messages for the same source line could solve this (when debug is turned on... which is likely to be the usual case). When run: "Average time spent with lock over 20 iterations is 38.05ms" I'm not happy about the signature in the joinpoint though: monitor-enter(void java.lang.Object.monitorenter(java.lang.Object)) yuck - other options? maybe: monitor-enter(java.lang.Object)
or maybe it should be lock(java.lang.Object) and unlock(java.lang.Object)
>> - handler(): the pointcut should take the form “lock/unlock(TypePat)” >> e.g. “lock(HelloWorld+)” so that specific types or type hierarchies can be >> targeted, especially with synchronized methods as opposed to blocks, without >> expensive residue. >It isn't quite the same so i can't do that - we know something about the >exception being caught at a handler join point, so can support >handle(TypePat) and reduce the residue. For a monitorentry/exit, we know >*nothing* about the argument to the call apart from that it is an 'Object'. >The argument is on the stack. So, you will have to rely on args() to pick it >out (and the residue that comes with it). So the forms will have to be >'lock()' and 'unlock()'. You do for synchronized methods which is probably the most frequent case. With my performance hat on I am concerned with the impact of an aspect that unecessarily advises every monitorenter/exit a code path I know to be very highly optimized. >or maybe it should be >lock(java.lang.Object) >and >unlock(java.lang.Object) I was about to say the same thing and we collided
Interesting discovery, the line number entries for a synchronized block indicate that the monitorentry and monitorexit both occur on the line where the word 'synchronized () {' occurs. Looks a little peculiar when expecting unlock() advice to occur (in terms of 'sourcelocation') on a line after the body of the block. Here is a woven method showing it - the advice always works as expected, it just appears a little unusual when asking the joinpoint for its source location. public void nonstaticM(); org.aspectj.weaver.MethodDeclarationLineNumber: length = 0x8 00 00 00 10 00 00 01 48 Code: Stack=5, Locals=2, Args_size=1 0: aload_0 1: dup 2: astore_1 3: monitorenter 4: getstatic #23; //Field java/lang/System.err:Ljava/io/PrintStream; 7: ldc #25; //String non-static method running 9: invokevirtual #31; //Method java/io/PrintStream.println:(Ljava/lang/String;)V 12: aload_1 13: invokestatic #48; //Method CombiningPCDs2.aspectOf:()LCombiningPCDs2; 16: aload_0 17: getstatic #50; //Field ajc$tjp_0:Lorg/aspectj/lang/JoinPoint$StaticPart; 20: invokevirtual #54; //Method CombiningPCDs2.ajc$before$CombiningPCDs2$1$3c1deed3:(LCombiningPCDs2$Foo;Lorg/aspectj/lang/J oinPoint$StaticPart;)V 23: monitorexit 24: goto 40 27: aload_1 28: invokestatic #48; //Method CombiningPCDs2.aspectOf:()LCombiningPCDs2; 31: aload_0 32: getstatic #57; //Field ajc$tjp_1:Lorg/aspectj/lang/JoinPoint$StaticPart; 35: invokevirtual #54; //Method CombiningPCDs2.ajc$before$CombiningPCDs2$1$3c1deed3:(LCombiningPCDs2$Foo;Lorg/aspectj/lang/J oinPoint$StaticPart;)V 38: monitorexit 39: athrow 40: return Exception table: from to target type 4 24 27 any 27 39 27 any LocalVariableTable: Start Length Slot Name Signature 0 41 0 this LCombiningPCDs2$Foo; LineNumberTable: line 17: 0 line 18: 4 line 17: 12 line 20: 40
> You do for synchronized methods which is probably the most frequent case. With > my performance hat on I am concerned with the impact of an aspect that > unecessarily advises every monitorenter/exit a code path I know to be very > highly optimized. I am kind of avoiding thinking about any of that transformation stuff at the moment - it will be a nightmare to get right - I just want the basics to work here with the new join points. But I take your point - on my run it occurred to me that there are other simple optimizations (for some of the common cases), for example in a non-static method an aload_0 as the instruction before indicates (un)locking on 'this'. A common pattern would seem to be combining it with 'withincode' when you can to limit the set of monitor calls you match on.
WRT line numbers both the weaveInfo message and gutter annotation for the execution PCD relate to the source line containing the method declaration, regardless of whether before and/or after advice is used, and not the closing brace so the sitution for sychronized() is not unique.
>A common pattern would seem to be combining it with 'withincode' when you can >to limit the set of monitor calls you match on. You are right although the semantics of "lock(Foo+)" and "lock() && within(Foo+)" are different. I realize that you are concentrating on phase one but I am concerned we don't restrict ourselves when deciding on the form of the new PCDs.
There are some problems with around() advice on these new join points. it is far too easy to get 'IllegalMonitorStateException' using around advice (Exception means: "Thrown to indicate that a thread has attempted to wait on an object's monitor or to notify other threads waiting on an object's monitor without owning the specified monitor.") And this relates to something Sam Pullara said in Jonas' blog: "JVMs will puke on them not being paired with the same method" The extraction of the MONITORENTER or MONITOREXIT instruction into a method which is then delegated to via proceed causes this problem. With around advice on lock() - If you don't proceed in the advice then the code will run, missing the MONITORENTER call. If you do put proceed in, you will fail IllegalMonitorState. With around advice on unlock() - If you don't proceed in the advice then the program will hang. If you do proceed the advice will loop continuously (I dont quite understand what is happening in this final case) The only thing that works is around advice on lock & unlock without proceed in either - skipping all the MONITOR instructions.
the restriction that the monitor calls need to be in the same method appears to be a SUN limitation. The woven code works fine on an IBM vm (both 1.5) - I've not tried other VMs. On the transform: - matthew suggested a way to preserve execution() is by extracting the body of a synchronized method into some ajc$ method and making the original a forwarding method. Ensure the execution() matches on the original. this would avoid intricate manipulation of the body of the method to ensure the advice for execution is called in the right place relative to the monitor instructions that will replace the synchronized method. - removing the synchronized bit and stashing it away will potentially affect all signature matching, since we will have to check for a modifier match and then check wherever we have stashed this 'bit' - presumably we only need transform synchronized methods if they are going to be matched by a lock() pointcut. Doing it for everything could adversely affect performance since synchronized methods will be implemented more efficiently than monitor based code. Keeping track of what we do/dont need to transform could be tricky.
The jpsig for this is going to be a little ugly, similar to handler which is: exception-handler(void A.<catch>(A$MyException)) we have lock(void java.lang.Object.<lock>(java.lang.Object)) unlock(void java.lang.Object.<unlock>(java.lang.Object)) the infrastructure (which I don't want to change right now...) wants a member signature in the parentheses, hence you see 'void A.<catch>(A$MyException)' in the handler case.
First bunch of code is all in - including quite a few tests. The implementation is discussed in the file SynchronizationTests - in there you can see the tasks involved in implementing it and it lists the outstanding tasks that remain before it can be made a non -X option. The critical ones are: - support around advice on these join points - ensure advice matching on method-execution against a synchronized method continues to execute within the lock rather than outside. For these kinds of current limitation new messages have been added so users will *know* what is going on if they choose to play with -Xjoinpoints:synchronization
Maybe I'm missing something but I just finished reading the JCIP book and it says that future VMs can and will do funky things with monitors, like handle them differently for contended/uncontended cases, change their scope or remove them completely when they are not needed. How will this affect the joinpoint execution model?
Being a -X option means we have put it out there for people to try - it may get promoted to being a core part of AspectJ, it may not - depending on the feedback we get. (Similarly, we have also added the option for -Xjoinpoints:arrayconstruction). Some of the problems and restrictions we have in the current implementation (like comment #13 with around advice - which is only a problem on some VMs and not others) may prove too much and we'll drop this feature. It may be that some of the other concerns you raise will also cause us to rethink what we do in this space - so if you have more information please provide it. We can't keep on top of every 'aspect' of the VM so to some degree rely on our users to supply information...
WRT Comment #17 I have not read the book (I assume it's this one http://www.javaconcurrencyinpractice.com/) but what you refer to probably concerns the actually implementation of a monitor which already varies considerably from one JVM to another. The AspectJ feature discribed in this report relies on the JVM specification. The join point(s) which the new pointcut matches are determined by that specification. As the implementation of Java concurrency evolves, like the introduction of java.util.concurrent, then we will have to evolve AspectJ.
(In reply to comment #17) > Maybe I'm missing something but I just finished reading the JCIP book and it > says that future VMs can and will do funky things with monitors, like handle > them differently for contended/uncontended cases, change their scope or remove > them completely when they are not needed. How will this affect the joinpoint > execution model? I think in his book Brian was referring to actual memory model and optimizations taken by hot spot JVM. Stuff ajc does just let you to hook up to the logical place where lock is being defined (monitorenter/monitorexit opcodes). So, even physical lock is skipped by JVM, advice on logical lock will be called, which is not necessary a bad thing. By the way, I wonder what kind of practical use cases AspectJ was considering when implementing this feature?
Yes, you make some valid points there. It could mean we will have to clarify the difference for end users in our doc: for example, just because their advice is called won't necessarily mean the VM is taking a lock. Some examples of usage are in the powerpoint slides attached to this bug. “time taken to acquire the lock” requires either: before and after the lock taken around the lock being taken “time spent with the lock (ownership)” Advice after it is taken and advice before/after it is released “change lock implementation” Maybe to switch to distributed locks of some kind around advice on lock(), unlock()
I've been looki ng trough slides and would like to make some remarks in regards to synchronized methods. One of the slides says that it is complicated to do the full implementation for synchronized methods. Such conclusion is probably based on BCEL experience, since it should be rather trivial to introduce new try/finally block for the whole method body using ASM. Another option (widely used by AW) is to rename existing method and create new non-syncronized method that would have code for all the advices (including around, etc) and will call renamed method appropriately. This option is probably easier to implement with BCEL.
> Such conclusion is probably based on BCEL experience, since it should be > rather trivial to introduce new try/finally block for the whole method body > using ASM. this was with respect to managing the ordering of all the entries in the exception table. it is not hard to add a simple finally block, but it is hard to add in and not adversely disturb the rest of the exception table because AspectJ itself currently provides minimal support for prioritizing exceptions appropriately - so we don't mess with that table if we don't have to (there are numerous bugs over the years related to making mistakes in that table) - AspectJ needs an overhaul in this area. My statement was not related to the mechanism used to manipulate the bytecode. > Another option (widely used by AW) is to rename existing method and create new > non-syncronized method that would have code for all the advices (including > around, etc) and will call renamed method appropriately. This option is > probably easier to implement with BCEL. of course - but that will have larger ramifications through AJ, in terms of making sure join points in the existing method look like they occurred in the scope of the original method (that's what the user would expect) and being careful for any new joinpoints we introduce into the new forwarding method. Hence, we haven't done that right now because I was in a hurry - when I have more time I may investigate this option - possibly depends what we decide to do for the around advice problem.
Hi all. I have a quick question about the current -X implementation of this pointcut in ajc. What syntax did you finally decide on? Is it "lock(x)" or "lock() && args(x)"? Also, are any values bound to this/target/args? Do you allow around advice on those pointcuts? (I know, it could work but it probably does not make sense very often to do this in an unstructured manner.) I am just asking because I implemented monitorenter/exit pointcuts in abc a couple of weeks ago and before I release them I want to make sure that the syntax and semantics are in sync with your's.
Hi all. Matthew and I had some discussion on the aspectj-users list recently about this warning message that ajc generates: advice matching the synchronized method shadow 'method-execution(void MonitorAjc$Test.foo())' will be executed outside the lock rather than inside (compiler limitation) Apparently this only happens when -Xjoinpoints:synchronization is enabled. Matthew mentioned in his posting that ajc apparently in this case restructures methods of the form... synchronized void foo() {} ... to methods of the form ... void foo() { synchronized(this){} } ... which then leads to the fact that when using an advice "before(): execution(synchronized * *.*(..)) ..." code is woven *before* the synchronized block, which then in turn leads to the warning. So what I don't understand about this behavior is: Why do you need this restructuring at all? I guess, right now, this is done to capture locks that are taken when entering synchronized methods but those locks can always be exposed the following way: before(Object o): execution(synchronized * *.*(..)) && this(o) && scope() { //locked o } after(Object o): execution(synchronized * *.*(..)) && this(o) && scope() { //locked o } So why should lock(*) capture synchronized method executions at all? I think it should only match monitorenter statements, not more.
(In reply to comment #25) > So why should lock(*) capture synchronized method executions at all? I think it > should only match monitorenter statements, not more. Please read the design notes. Remember that private synchronized void println1 () { System.out.println("Hello World!"); } and private void println2 () { synchronized(this) { System.out.println("Hello World!"); } } are semantically identical. If we don't match synchronized methods (which have an implicit monitor_enter/exit) then most of the requirements for this enhancement would not be met.
(In reply to comment #26) > Please read the design notes. Remember that > > private synchronized void println1 () { > System.out.println("Hello World!"); > } > > and > > private void println2 () { > synchronized(this) { > System.out.println("Hello World!"); > } > } Ah, ok that explains where this comes from... Yet, I don't agree with the way this is currently implemented. You can compare this situation quite a bit with the following one: You have a pointcut execution(* *(..)) and then this code: void foo() { doSomething(); } compared to: void foo() { bar(); } private void bar() { doSomething(); } In the former case, the pointcut would match at two shadows in the latter at three, although the codes are functionally equivalent. All this to say that AspectJ never cared about functional equivalence. AspectJ matching was always defined in term of structure. Hence, I don't think it's a good thing to now break with that rule for synchronization pointcuts. I would instead just rely on programmers being capable of writing two pointcuts, one capturing calls of synchronized methods and one capturing synchronized blocks. That way there would be no need no more for any magic in the backend and also the problem of non-synchronized advice execution would go away.
> Ah, ok that explains where this comes from... Yet, I don't agree with the way > this is currently implemented. You can compare this situation quite a bit with > the following one: > You have a pointcut execution(* *(..)) and then this code: > void foo() { > doSomething(); > } > compared to: > void foo() { > bar(); > } > private void bar() { > doSomething(); > } > In the former case, the pointcut would match at two shadows in the latter at > three, although the codes are functionally equivalent. All this to say that > AspectJ never cared about functional equivalence. AspectJ matching was always > defined in term of structure. Hence, I don't think it's a good thing to now > break with that rule for synchronization pointcuts. I would instead just rely > on programmers being capable of writing two pointcuts, one capturing calls of > synchronized methods and one capturing synchronized blocks. That way there > would be no need no more for any magic in the backend and also the problem of > non-synchronized advice execution would go away. No, that is an incorrect analogy. Pointcuts match join points (not shadows which are woven to facilitate advice). The lock() and unlock() pointcuts select the event where the monitor associated with an object is acquired and released respectively regardless of how it is implemented either by the programmer or the JVM. Why should AspectJ match synchronized blocks but not methods? How would you implement the use case examples provided if you only matched a (small) subset of the join points? >AspectJ matching was always defined in term of structure. No, it is defined in terms of events. >I would instead just rely >on programmers being capable of writing two pointcuts, one capturing calls of >synchronized methods and one capturing synchronized blocks. Please read the design documentation. You _cannot_ reliably weave a synchronized method without the transformation because you may not be able to weave the method call join point shadow. Besides as a programmer if I write "lock()" I expect AspectJ to match all the joint points no matter how tricky that might be not "part of this problem is left to the reader"!
(In reply to comment #28) > How > would you implement the use case examples provided if you only matched a > (small) subset of the join points? You can always use the execution pointcuts to match on synchronized method executions. So you could definitely implement all use cases with lock/unlock only matching on synchronized *blocks*. > Please read the design documentation. You _cannot_ reliably weave a > synchronized method without the transformation because you may not be able to > weave the method call join point shadow. Besides as a programmer if I write > "lock()" I expect AspectJ to match all the joint points no matter how tricky > that might be not "part of this problem is left to the reader"! But then ajc should rather rewrite the pointcut instead of the base code, I believe. An advice of the form... before(Foo f): lock(f) { ... ... should be interpreted internally as ... before(Foo f): monitorenter(f) || execution(* *.(..)) && this(f) { ... (where monitorenter(f) only matches synchronized blocks, not method executions). That way there would no need for restructuring IMHO. Eric
(In reply to comment #29) > before(Foo f): monitorenter(f) || execution(* *.(..)) && this(f) { ... There I actually meant: before(Foo f): monitorenter(f) || execution(synchronized * *.(..)) && this(f) { ...
Please tell me how "before() : lock()" is equivalent to "before() : execution(synchronized ...)". In the former the advice is called __before__ monitorenter while in the latter it is called __after__. This is because the JVM takes the object lock (read design notes pretty please) __before__ entering the body of the method. As an AspectJ programmer if I write "before() : lock()" I mean __before__!
(In reply to comment #31) > In the former the advice is called __before__ > monitorenter while in the latter it is called __after__. Ah, I think that's the crucial point I was missing. Thanks for the clarification. Ok, now I agree with you.
The 1.5.3 implementation of this experimental feature is providing "0" as the line number (thisJoinPointStaticPart.getSourceLocation()) for all synchronized methods, the blocks appear to return the correct line number. For example I get: - com.surelogic.dynamic.test.IntrinsicLocking@f0d7b0 at IntrinsicLocking.java:0 - java.lang.Object@9319f9 at IntrinsicLocking.java:22 for a call to the method (within a class called IntrinsicLocking.java): 21 synchronized void lock() { 22 synchronized (lock) { 23 System.out.println("lock()"); 24 } 25 }
We (the abc group) are currently looking into the problem of avoiding the restructuring of synchronized methods that's currently necessary at the moment, for the following reasons: 1.) it leads to advice code induced by execution-joinpoints being executed outside the lock (ajc at the moment gives a warning in those cases) 2.) we saw benchmarks that run about 10% slower only because synchronized methods were replaced by monitorenter/exit statements We do have an implementation scheme that would avoid generating monitorenter/exit statements and hence would yield better performance. Also, this would be capable of consistently executing the advice code for execution-joinpoints inside the lock. *However* it would forbid the use of around-advice in combination lock/unlock pointcuts. The synchronized method foo()... synchronized void foo() { //body } ... would be replaced by: public void foo() { //(1) original$foo(); //(2) //(3) } private synchronized void original$foo() { //(4) //original method body //(5) } This has the following implications: * we would not need to introduce any monitorenter/exit statements which means we would presumably get better performance * we could weave advice code before monitorenter, this would go to location (1) * we could weave advice code after monitorexit, this would go to location (3) * we could weave advice code after monitorenter, this would go to location (4) * we could weave advice code before monitorexit, this would go to location (5) * code matching on the original method via execution-pointcuts would have to be woven within original$foo() instead of foo() So my question is whether anybody does actually have a compelling use case for around advice on those joinpoints. To me it does not make sense to just blindly remove synchronization so this might not be a good use case. Omitting a monitorenter but not the matching monitorexit would be an error anyway, equally the other way around. Proceeding *more* than once also seems to make no sense and proceeding exactly once can always be implemented via before/after. So maybe it would make sense to just not allow messing around with "around", if that buys us a more efficient and consistent solution?
What the status for this request? I have use case for it. I would like to write a debuging apects that help to identify non thread safe code. I want to write an aspect that trace the write access to members done by two different thread. For each member updated/accessed by two different thread, I also want to list the object used to synchronize those access.
Hi Gilles. You can enable lock() and unlock() pointcuts using -Xjoinpoints:synchronization. I have just released RacerAJ, a set of aspects that finds potential for data races using this feature. Maybe our implementation can assist you in getting the syntax right: http://www.bodden.de/tools/raceraj/ I guess the crucial aspect would be that one here: http://www.bodden.de/wordpress/wp-content/uploads/2008/05/Locking.aj Hope that helps.
I would like to have this too. I could work nicely together with the jcip (from the Java Concurrency in Practice book) annotations.
(In reply to comment #37) > I would like to have this too. I could work nicely together with the jcip (from > the Java Concurrency in Practice book) annotations. Hi Wim. As I wrote, pointcuts for monitorenter/exit are available through the command line flag -Xjoinpoints:synchronization. As I found out, the current support for monitorenter/exit suffices for checking all properties that I have found in this book so far. I encourage you to try this out and see if there are actually any cases where these pointcuts would be insufficient.
hi there, Don't know the curr. status of this issue, but anyway let me share a few thoughts (this is legal, right ? ;)) Why you all here consider "synchronized" over "java.util.concurrent.locks.Lock"? I read all comments here but found no words about Lock/ReentrantLock. One solid reason for ReentrantLock's benefit - ability to track reentrancy(ReentrantLock.getHoldCount()). I believe there are more other benefits as well. Thanks for your time! P.S. One little remark: as for me matching(after/before) either "synchronized" jp or Java 5's "Lock.lock/unlock" jp doesn't bring big value, main achievement would be an exposure of the state which "synchronized"/"Lock" are aimed to guard.
Hi. (In reply to comment #39) > Why you all here consider "synchronized" over > "java.util.concurrent.locks.Lock"? I think the reason is that already today you can handle ReentrantLock sinmply though the language mechanisms that AJ provides. After all, it's just a regular API with regular method calls that you can go "around" if you feel like it. That is different from synchronized blocks, of course, which are built into the language.
