<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div><blockquote type="cite" class=""><div class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" class="">Therefore, in the current webrev, I've disabled folding of accesses to<br class="">all non-stable fields.<br class=""></blockquote>Please do not do that, it will be a huge regression in term of perf at least for the language runtimes i maintain<br class="">(and i suppose for Nashorn, JRuby or Groovy), Here are the perf assertions i use routinely,<br class="">static final method handle is considered as constant, final fields (even not static) of VM anonymous class is considered as truly final.<br class=""><br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">I fully understand your concerns. And thank you for sharing your asserts!</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><div><br class=""></div><div>The folding within the compiler should be handled differently since the concerns are different and removing it will likely cause invokeynamic performance to collapse.  The compiler is only folding final instance fields that are part of a chain of constants, so the main concern is whether an object has been published before it’s been fully constructed or if someone is going to use reflection to modify a final field after the object has been published.  The trust_final_non_static_fields logic in ciField.cpp is calling out classes which we control that shouldn’t violate those rules and that are important for performance.  So I think it should be left as is.  It really doesn’t relate to the issue at hand I think.</div><div><br class=""></div><div>Also you’ve removed folding of static final fields which would be a very bad thing to do.</div><div><br class=""></div><div>ciField has some caching logic for _known_to_link_with_get/set and if you are adding a ciMethod* argument you also need to cache the method that was checked last time as well.</div><div><br class=""></div><div>tom</div><br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Let's see what others think about how to go about this problem. So far the following options were explored</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">- bail out compilation of non-compliant methods;</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">- enforce JVMS conformance for all classes and keep constant folding enabled;</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">- enforce JVMS conformance only for classes with _major_version >=52 and completely disable constant folding.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">I'm not sure which option is the best. Also, there might be other options as well.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Thank you and best regards,</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Zoltan</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" class="">We could relax that criteria, e.g., by disabling folding only for those<br class="">field that were declared in a class with _major_version < 52. But it<br class="">would be great if you could give me some feedback before I look into<br class="">that. Thank you very much in advance!<br class=""><br class="">Updated webrev:<br class=""><a href="http://cr.openjdk.java.net/~zmajo/8157181/webrev.05/" class="">http://cr.openjdk.java.net/~zmajo/8157181/webrev.05/</a><br class=""><br class="">Testing: JPRT in progress.<br class=""><br class="">Best regards,<br class=""><br class=""><br class="">Zoltan<br class=""></blockquote>Rémi<br class=""><br class=""><blockquote type="cite" class=""><blockquote type="cite" class="">Thank you and best regards,<br class=""><br class=""><br class="">Zoltan<br class=""><br class=""><blockquote type="cite" class="">Best regards,<br class="">Vladimir Ivanov<br class=""><br class=""><blockquote type="cite" class="">I agree with you, alternatively we can propose a more generic fix (fix<br class="">the interpreter and the compilers as well). The fix would most likely<br class="">affect field resolution in LinkResolver::resolve_field() around here:<br class=""><a href="http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/interpreter/linkResolver.cpp#l910" class="">http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/interpreter/linkResolver.cpp#l910</a><br class=""><br class=""><br class=""><br class="">Changing resolve_field() to throw an IllegalAccessError for<br class="">inappropriate field access will propagate the exception to the<br class="">interpreter. Changing ciField::will_link() will most likely kill (some)<br class="">compilations (if, e.g., fields are linked through<br class="">ciField::will_link()).<br class=""><br class="">I'd prefer to take the second option (changing field resolution), but<br class="">there might be some disadvantages related to option I might be<br class="">overseeing.<br class=""></blockquote><br class=""><br class=""><br class=""><blockquote type="cite" class="">Thank you!<br class=""><br class="">Best regards,<br class=""><br class=""><br class="">Zoltan<br class=""><br class=""><br class=""><blockquote type="cite" class=""><blockquote type="cite" class="">More details about the failure are here [3].<br class=""><br class="">With the patch applied, the program always completes as it is<br class="">expected<br class="">by Jython, no matter if we use -Xint, -Xcomp, or -Xmixed (because we<br class="">always interpret methods non-conforming with the VM spec).<br class=""><br class="">Here is the updated webrev:<br class=""><a href="http://cr.openjdk.java.net/~zmajo/8157181/webrev.01/" class="">http://cr.openjdk.java.net/~zmajo/8157181/webrev.01/</a><br class=""></blockquote>Bailing out the whole compilation in C2 is even less appropriate. It<br class="">should generate an uncommon trap for such accesses instead (see<br class="">Parse::do_field_access).<br class=""><br class="">Best regards,<br class="">Vladimir Ivanov<br class=""><br class="">[1]<br class=""><a href="http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/c1/c1_GraphBuilder.cpp#l1595" class="">http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/c1/c1_GraphBuilder.cpp#l1595</a></blockquote></blockquote></blockquote></blockquote></blockquote></blockquote></div></blockquote></div><br class=""></body></html>