On Oct 14, 2014, at 8:54 PM, Vladimir Ivanov <vladimir.x.iva...@oracle.com> wrote: > Paul, > > Thanks for the feedback! > > Updated version: > http://cr.openjdk.java.net/~vlivanov/8059877/webrev.01/ > >>> http://cr.openjdk.java.net/~vlivanov/8059877/webrev.00/ >>> https://bugs.openjdk.java.net/browse/JDK-8059877 >>> >> >> Generally looks ok. >> >> - MethodHandleImpl >> >> 786 if (wrapper.unblock()) { >> 787 // Reached invocation threshold. Replace >> counting/blocking wrapper with a reinvoker. >> 788 wrapper.isActivated = false; >> >> If unblock() returns true then isActivated is already false. > Fixed. > >> At the expense of another box you could use an AtomicBoolean with >> compareAndSet if you want to really guarantee at most one unblocking, >> although the box can be removed if the boolean is changed to 'int' and >> Unsafe is used to CAS. > I considered that, but I decided not to go that route. GuardWithTest is a > very common combinator, so footprint overhead could be noticeable. There's no > need to guarantee uniqueness of unblocking operation. The operation is > idempotent, so no problems performing it multiple times. What I try to > achieve with the flag is avoid pathological situation when some thread > continuously updates the form.
Ok. > >> I dunno if strengthening the visibility of MethodHandle.form by stamping in >> a memory fence after the following will help >> >> 792 wrapper.updateForm(lform); >> >> e.g. using Unsafe.fullFence. > I think the fence should be there. It won't help guarantee the update is > visible everywhere though. But it is expected. > I see you added it to MethodHandle.updateForm, i was incorrectly assuming you just wanted to add in the context of BlockInliningWrapper after the update call, so not all updates incur the full barrier cost for other calls to updateForm, but it makes sense in light of the ISSUE comment. Actually i forgot MethodHandle.form is marked final!, i better understand some of your comments now, so visibility is even more restricted that i previously thought. This is a good example to add to our "stomping on a final field" discussion, i think here it is definitely a very special case with a careful dance between updating and inlining (updateForm is also called by a direct method handle to a static method or field, such a handle initially holds a form with a check to initialize the class and after that occurs the handle is updated with a new form without the check). >> Perhaps isUnblocked is a better name than isActivated since there is no need >> to set the initial value and it tracks the same value as that returned from >> unblock? > Done. > 765 isUnblocked = false; Should be "isUnblocked = true". 756 MethodHandle newTarget = target.asType(newType); 757 return asTypeCache = isUnblocked ? make(newTarget) 758 : newTarget; // no need for a wrapper anymore Should be "return asTypeCache = !isUnblocked ? ? make(newTarget)" No need for another review round. >> Also while on the subject of naming perhaps consider changing >> MethodTypeForm.LF_DELEGATE_COUNTING to ...LF_DELEGATE_BLOCK_INLINING? > Done. > >> When DONT_INLINE_THRESHOLD > 0 and DONT_INLINE_THRESHOLD == >> COMPILE_THRESHOLD will that trigger both compilation of a >> BlockInliningWrapper's form and unblocking? I guess there is some fuzziness >> depending on concurrent execution. > No problems expected in this scenario - COMPILE_THRESHOLD updates LambdaForm > entry point and unblocking operates on a method handle. > >> I am just wondering if there is any point compiling a BlockInliningWrapper's >> form if DONT_INLINE_THRESHOLD is expected to be ~ the same as >> COMPILE_THRESHOLD. Probably not terribly important especially if the >> JIT-compiler control approach replaces it. > I don't see any value in keeping BlockInliningWrapper interpreted. > It would allow to avoid LambdaForm.forceInline flag, but there should be a > special case to skip compilation (in LambdaForm::checkInvocationCounter()). > Moreover, if we get rid of LambdaForm interpreter, we will need to precompile > BlockInliningWrapper again. > Ok. Paul.
signature.asc
Description: Message signed with OpenPGP using GPGMail
_______________________________________________ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
