<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">I'm gathering up issues from old mail, and found the incomplete exchange below on the mlvm dev list.<div><br></div><div>I've followed your advice and removed the "protected" keyword from CallSite fields. The accessors are sufficient.</div><div><br></div><div>I'm also going to remove the single-inheritance mixin pattern which you objected to, since IBM has also objected to it (on the EG). Basically, I'm going to have the JVM special-case MethodHandle instead of a superclass sun.dyn.MethodHandleImpl.</div><div><br></div><div>(My English teacher in high school used to say, "kill your darlings", meaning that, if you have a bit of writing you think particularly clever, you probably need to edit it.)</div><div><br></div><div>I've raised the issues about target invalidation with the EG. Are there other issues in the exchange below that we should call out?</div><div><br></div><div>Thanks,</div><div>-- John<br><div><br><div>Begin forwarded message:</div><br class="Apple-interchange-newline"><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" color="#000000" style="font: 12.0px Helvetica; color: #000000"><b>From: </b></font><font face="Helvetica" size="3" style="font: 12.0px Helvetica">Rémi Forax <<a href="mailto:forax@univ-mlv.fr">forax@univ-mlv.fr</a>></font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" color="#000000" style="font: 12.0px Helvetica; color: #000000"><b>Date: </b></font><font face="Helvetica" size="3" style="font: 12.0px Helvetica">September 18, 2008 4:40:34 PM PDT</font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" color="#000000" style="font: 12.0px Helvetica; color: #000000"><b>To: </b></font><font face="Helvetica" size="3" style="font: 12.0px Helvetica">Da Vinci Machine Project <<a href="mailto:mlvm-dev@openjdk.java.net">mlvm-dev@openjdk.java.net</a>></font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" color="#000000" style="font: 12.0px Helvetica; color: #000000"><b>Subject: </b></font><font face="Helvetica" size="3" style="font: 12.0px Helvetica"><b>Re: Ask for a patch review: CallSite.target is not volatile</b></font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" color="#000000" style="font: 12.0px Helvetica; color: #000000"><b>Reply-To: </b></font><font face="Helvetica" size="3" style="font: 12.0px Helvetica">Da Vinci Machine Project <<a href="mailto:mlvm-dev@openjdk.java.net">mlvm-dev@openjdk.java.net</a>></font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div> </div><div>John Rose a écrit :<br><blockquote type="cite">On Sep 13, 2008, at 7:27 AM, Rémi Forax wrote:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite">currently CallSite.target is not volatile, so if an invokedynamic <br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">bootstrap<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">method calls setTarget() in one thread, other threads may not see the<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">new target.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Proposed changes :<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> - make target volatile<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> - use unsafe.putOrdered to set the field in a more lightweight way.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Hmm... The EDR is ambiguous about this, and needs to be tightened <br></blockquote><blockquote type="cite">up. It does say that the bulk invalidation API is intended to <br></blockquote><blockquote type="cite">provide a global interlock, and gives some freedom to the propagation <br></blockquote><blockquote type="cite">of setTarget effects. What the EDR says about "next call" refers to <br></blockquote><blockquote type="cite">a global ordering that doesn't really exist in the JVM apart from <br></blockquote><blockquote type="cite">synchronization.<br></blockquote><blockquote type="cite"><br></blockquote>yop, i have seen that. I'm currently not able to provide<br>that requirement for the backport.<br><blockquote type="cite">Native JVM implementations don't require target to be explicitly <br></blockquote><blockquote type="cite">volatile. In fact, individual threads need the freedom to cache the <br></blockquote><blockquote type="cite">target, e.g., as a loop invariant for a fast-path version of a loop. <br></blockquote><blockquote type="cite">The important thing to optimize here is the speed of *reading* the <br></blockquote><blockquote type="cite">target field. Writes can be as slow as we want (I use a native <br></blockquote><blockquote type="cite">method call).<br></blockquote><blockquote type="cite"><br></blockquote>The problem is if a new class is loaded by another thread,<br>I may want to change the target for any other threads.<br>If you cache the target, it will not change when a new class is discovered.<br>i.e the target will change in RAM but not in the local cache.<br><br>ConcurrentLinkedQueue<Data> queue=...<br><br>thread1:<br>for(Data data:queue) {<br> // invokedynamic call using data<br>}<br><br><br>thread2:<br>queue.add(new DataSubClass());<br>// here the classloader load DataSubClass and<br>// defeat the optimisation done at<br>// invokedynamic call site.<br><br><br><blockquote type="cite">All this is independent of whether the code says "volatile" or not on <br></blockquote><blockquote type="cite">the field, but it may be better not to mark it volatile, since that <br></blockquote><blockquote type="cite">seems to make a guarantee about propagation of setTarget effects.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Note that the JVM reads the target field in a specialized way, not <br></blockquote><blockquote type="cite">necessarily through a bytecode. Also, the writes to the target field <br></blockquote><blockquote type="cite">go through an accessor, so once again, the volatility of the field <br></blockquote><blockquote type="cite">does not necessarily matter. <br></blockquote>not agree, if the accessor is inlined, target can be not reloaded from <br>the RAM.<br><blockquote type="cite"> Except to the backport. For the <br></blockquote><blockquote type="cite">backport, I guess the volatile bit is needed. So, yes, that's a good <br></blockquote><blockquote type="cite">change.<br></blockquote><blockquote type="cite"><br></blockquote>Yes.<br><blockquote type="cite">Maybe the right thing to do is move target into a mixin-style <br></blockquote><blockquote type="cite">superclass, java.dyn.impl.CS, and have it be volatile in the backport.<br></blockquote><blockquote type="cite"><br></blockquote>I'am not sure you can avoid the volatile read.<br><blockquote type="cite">This takes me to the second proposed change.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite"> - change alls field visibility from protected to private because :<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> 1) protected fields appear in the JavaDoc,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> so another implementation of CallSite (like the one currently<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">use by the backport)<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> is not allowed.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> 2) caller, name and type are final so caller(), name() and type()<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> can be used instead.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> subclasses can override checkTarget() to do nothing, in that<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> case setTarget() is equivalent to change the target field.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> So I see no reason to declare this fields protected.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I want the CallSite to be usable two ways:<br></blockquote><blockquote type="cite">1. created by the JVM to reify invokedynamic instructions<br></blockquote><blockquote type="cite">2. creatable by Java code to simulate invokedynamic sites interpretively<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Therefore, it's important to allow Java code to make its own flavor <br></blockquote><blockquote type="cite">of CallSite. Hence the protected methods & fields.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">This could also be done with a CallSite interface, but it seems <br></blockquote><blockquote type="cite">easier to me (and to other JVM vendors I think) to make it a concrete <br></blockquote><blockquote type="cite">class.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Therefore, let's keep the protected stuff for now, unless we decide <br></blockquote><blockquote type="cite">to make CallSite into an interface.<br></blockquote><blockquote type="cite"><br></blockquote>You don't need protected field if you have accessors. And currently<br>those accessors exist.<br><blockquote type="cite">I want to factor the 292 API into common classes, including the <br></blockquote><blockquote type="cite">public ones, and non-standard implementation-specific classes, one <br></blockquote><blockquote type="cite">version for each kind of JVM and one for the backport. The <br></blockquote><blockquote type="cite">java.dyn.impl package contains the implementation-specific classes. <br></blockquote><blockquote type="cite">(This name will change to go outside of the java.* hierarchy.) When <br></blockquote><blockquote type="cite">a public type needs to mix in fields or methods, it does so via <br></blockquote><blockquote type="cite">inheritance from an implementation superclass. Switching between <br></blockquote><blockquote type="cite">backport and native implementations will therefore require <br></blockquote><blockquote type="cite">adjustments to boot class path or the installed rt.jar.<br></blockquote><blockquote type="cite"><br></blockquote>Why not use a static final field, like the anonk code already does ?<br>and detect at runtime if there is a native impl or if the backport<br>must be used.<br><blockquote type="cite">Why use mixins? Although such pluggable implementations are more <br></blockquote><blockquote type="cite">often done via delegation or subclassing, the performance <br></blockquote><blockquote type="cite">requirements of method handles requires a more aggressive (less <br></blockquote><blockquote type="cite">flexible) pattern of single-inheritance mixins.<br></blockquote><blockquote type="cite"><br></blockquote>I think a static final field is a better option.<br><blockquote type="cite">-- John<br></blockquote><blockquote type="cite"><br></blockquote>Rémi<br>_______________________________________________<br>mlvm-dev mailing list<br><a href="mailto:mlvm-dev@openjdk.java.net">mlvm-dev@openjdk.java.net</a><br>http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev<br></div></div><br><div><br></div><div><br></div><div><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><br><div>Begin forwarded message:</div><br class="Apple-interchange-newline"><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" color="#000000" style="font: normal normal normal 12px/normal Helvetica; color: rgb(0, 0, 0); "><b>From: </b></font><font face="Helvetica" size="3" style="font: normal normal normal 12px/normal Helvetica; ">John Rose <<a href="mailto:John.Rose@Sun.COM">John.Rose@Sun.COM</a>></font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" color="#000000" style="font: normal normal normal 12px/normal Helvetica; color: rgb(0, 0, 0); "><b>Date: </b></font><font face="Helvetica" size="3" style="font: normal normal normal 12px/normal Helvetica; ">September 18, 2008 5:40:06 PM PDT</font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" color="#000000" style="font: normal normal normal 12px/normal Helvetica; color: rgb(0, 0, 0); "><b>To: </b></font><font face="Helvetica" size="3" style="font: normal normal normal 12px/normal Helvetica; ">Da Vinci Machine Project <<a href="mailto:mlvm-dev@openjdk.java.net">mlvm-dev@openjdk.java.net</a>></font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" color="#000000" style="font: normal normal normal 12px/normal Helvetica; color: rgb(0, 0, 0); "><b>Subject: </b></font><font face="Helvetica" size="3" style="font: normal normal normal 12px/normal Helvetica; "><b>Re: Ask for a patch review: CallSite.target is not volatile</b></font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" color="#000000" style="font: normal normal normal 12px/normal Helvetica; color: rgb(0, 0, 0); "><b>Reply-To: </b></font><font face="Helvetica" size="3" style="font: normal normal normal 12px/normal Helvetica; ">Da Vinci Machine Project <<a href="mailto:mlvm-dev@openjdk.java.net">mlvm-dev@openjdk.java.net</a>></font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div></div><div>On Sep 18, 2008, at 4:40 PM, Rémi Forax wrote:<br><br><blockquote type="cite">The problem is if a new class is loaded by another thread,<br></blockquote><blockquote type="cite">I may want to change the target for any other threads.<br></blockquote><blockquote type="cite">If you cache the target, it will not change when a new class is <br></blockquote><blockquote type="cite">discovered.<br></blockquote><blockquote type="cite">i.e the target will change in RAM but not in the local cache.<br></blockquote><br>Do we agree that an cached old target is not a correctness issue?<br><br>A system which an have out-of-date targets that has to be robust <br>against such changes even without a volatile variable. The target <br>method itself must include whatever version checks are needed for <br>correctness.<br><br>What volatile might buy you is a slightly better guarantee that the <br>new target will be seen earlier. But, if the old target has a <br>correctness check, it will presumably have a slow path to clean up <br>when a problem is seen, and this in turn, if it changes the state of <br>memory, will cause the thread to reload even a non-volatile target <br>field.<br><br>If a language system use targets that do not perform version checks <br>(against data structures with variable semantics, such as open <br>inheritance hierarchies), it will need to create some sort of <br>handshake to get out of the old code before the new code is loaded. <br>(See SystemDictionary::add_to_hierarchy in the JVM.) This requires a <br>safepoint mechanism of some sort to make sure that all threads are <br>blocked from entering call sites to old targets, and/or roll forward <br>out of such call sites.<br><br>Generally, only the JVM knows enough about each thread to make this <br>happen, hence the need for a bulk invalidation API. I'm thinking <br>that the language runtime will do the following to implement a change <br>to an unchecked quasi-invariant:<br>1. grab a lock on the variable global state (e.g., class hierarchy)<br>2. compute a limited set of call sites to invalidated, based on <br>language-specific dependency tables<br>3. call the 292 API to bulk-invalidate those call sites<br>4. make the change to the variable global state<br>5. eagerly patch invalidated call sites, if desired, and/or respond <br>to bootstrap requests (which can interact with the global lock)<br>6. release the lock<br>7. recover from any remaining consequences incrementally, as <br>bootstrap methods are called on invalidated call sites<br><br>Step 3 is magic and expensive. It may invalidate "innocent <br>bystander", call sites which are not actually invalid. It will at <br>least make sure that any pending dynamic calls at the invalid call <br>sites are fully executed, up to a global synchronization point that <br>it defines internally.<br><br>However, after step 3, there may well be old methods on the stack. <br>For example, a dynamic call might have completed to an outdated<br>It is up to the language runtime to cope with these. We come back to <br>the need to have self-verifying or self-correcting target methods.<br><br>In fact, the whole bulk invalidation API could be unnecessary, given <br>that target methods must always be self-verifying anyway. However, I <br>think it will simplify the self-verification tests, for some languages.<br><br>BTW, in HotSpot compiled methods are self-correcting, in that they <br>can be deoptimized at any time, replaced by interpreted code, which <br>is less optimal but more robustly correct. I'm trying to stay away <br>from defining such a mechanism above the level of bytecodes, because <br>I don't see how to define and implement it simply.<br><br>The "hook" you get now for this behavior is to generate an explicit <br>slow-patch check in the bytecodes. By that I mean a test which <br>always succeeds, until the quasi-invariant fails. The JIT is likely <br>to notice (from the profile) that the check "always" succeeds, and <br>compile it to an "uncommon trap", a dead-end in the compiled code <br>which deoptimizes and jumps back to the interpreter. The JIT does <br>not compile anything downstream of the uncommon trap, and so the <br>cleanup does not affect the optimization of the fast path. As long <br>as only the fast path is taken, the specialized JIT code rules.<br><br><blockquote type="cite"><blockquote type="cite">Note that the JVM reads the target field in a specialized way, not<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">necessarily through a bytecode. Also, the writes to the target field<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">go through an accessor, so once again, the volatility of the field<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">does not necessarily matter.<br></blockquote></blockquote><blockquote type="cite">not agree, if the accessor is inlined, target can be not reloaded from<br></blockquote><blockquote type="cite">the RAM.<br></blockquote><br>The point is, the accessor can have the required memory barrier <br>(e.g., from Unsafe, or an empty synch) without marking the field <br>volatile.<br><br><blockquote type="cite"><blockquote type="cite">Maybe the right thing to do is move target into a mixin-style<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">superclass, java.dyn.impl.CS, and have it be volatile in the <br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">backport.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite">I'am not sure you can avoid the volatile read.<br></blockquote>I'm not yet sure that the volatile read even does what you hope it <br>does! See above.<br><br><blockquote type="cite">You don't need protected field if you have accessors. And currently<br></blockquote><blockquote type="cite">those accessors exist.<br></blockquote>Right, except for CallSite.callerObject.<br><br><blockquote type="cite">Why not use a static final field, like the anonk code already does ?<br></blockquote><blockquote type="cite">and detect at runtime if there is a native impl or if the backport<br></blockquote><blockquote type="cite">must be used.<br></blockquote>That doesn't let you mix in JVM-specific fields like a superclass does.<br><br>Best wishes,<br>-- John<br>_______________________________________________<br>mlvm-dev mailing list<br><a href="mailto:mlvm-dev@openjdk.java.net">mlvm-dev@openjdk.java.net</a><br>http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev<br></div><div><br></div></div></div></div></div></body></html>