<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Roman,<div class=""><br class=""></div><div class="">The first thing to do in this case is to search JBS for any known issue in this test. By searching for the test name for instance you will find this issue: JDK-8199515</div><div class=""><a href="https://bugs.openjdk.java.net/issues/?jql=text ~ "java/lang/invoke/condy/CondyInterfaceWithOverpassMethods.java"" class="">https://bugs.openjdk.java.net/issues/?jql=text%20~%20%22java%2Flang%2Finvoke%2Fcondy%2FCondyInterfaceWithOverpassMethods.java%22</a></div><div class=""><br class=""></div><div class="">Obviously, without more information about your particular failure you can not be 100% sure that this is the same bug, but it's a good pice of information to provide to the Oracle engineer who you ask to look at your failure. If there is an error in your tier1 run you should NOT push unless someone that has investigated the issue gives you a GO.</div><div class=""><br class=""></div><div class="">In this case your failure is a known issue that is being worked on, JDK-8199515, and you can go ahead and push provided the change is reviewed and all those other checkboxes are ticked.</div><div class="">/Jesper<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On 13 Mar 2018, at 16:40, Roman Kennke <<a href="mailto:rkennke@redhat.com" class="">rkennke@redhat.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">The test came back with the below. Doesn't strike me as being related to<br class="">this change.<br class=""><br class="">What to do now? Push it anyway? Re-submit the test? (If so, how?)<br class=""><br class="">Roman<br class=""><br class="">Build Details: 2018-03-13-1330406.roman.source<br class="">1 Failed Test<br class="">Test<span class="Apple-tab-span" style="white-space:pre"> </span>Tier<span class="Apple-tab-span" style="white-space:pre"> </span>Platform<span class="Apple-tab-span" style="white-space:pre"> </span>Keywords<span class="Apple-tab-span" style="white-space:pre"> </span>Description<span class="Apple-tab-span" style="white-space:pre"> </span>Task<br class="">java/lang/invoke/condy/CondyInterfaceWithOverpassMethods.java <span class="Apple-tab-span" style="white-space:pre"> </span>tier1<br class="">windows-x64 <span class="Apple-tab-span" style="white-space:pre"> </span>bug8186046 othervm testng <span class="Apple-tab-span" style="white-space:pre"> </span>Error: failed to clean up files<br class="">after test <span class="Apple-tab-span" style="white-space:pre"> </span>task<br class="">Mach5 Tasks Results Summary<br class=""><br class=""> NA: 0<br class=""> FAILED: 0<br class=""> PASSED: 74<br class=""> UNABLE_TO_RUN: 0<br class=""> KILLED: 0<br class=""> EXECUTED_WITH_FAILURE: 1<br class=""> Test<br class=""><br class=""> 1 Executed with failure<br class=""> jdk_open_test_jdk_tier1-windows-x64-66 Results: total: 1775,<br class="">passed: 1774; failed: 1<br class=""><br class=""><br class=""><br class=""><blockquote type="cite" class="">Hi Erik & Per,<br class=""><br class="">yes, I agree with getting rid of Runtime1::arraycopy(). Let's take it to<br class="">another RFE.<br class=""><br class="">Let's try the newfangled submit repo and push (mostly) my first patch :-)<br class=""><br class="">Thanks for reviewing!<br class="">Cheer, Roman<br class=""><br class=""><br class=""><blockquote type="cite" class="">Hi Roman,<br class=""><br class="">Ideally I'd like to get rid of Runtime1::arraycopy. It seems like a<br class="">medium slow path of C1 never used by any platforms other than S390 and<br class="">PPC, which look like the only platforms that lack a generic arraycopy<br class="">stub from the StubRoutines. I think that S390 and PPC could easily just<br class="">jump to the C1 arraycopy stub instead and take the slowpath to the<br class="">normal native arraycopy that enters through the JNI interface instead.<br class=""><br class="">If the performance of this C1 arraycopy path on S390 and PPC are<br class="">crucial, then they should probably implement stubs for it like the other<br class="">platforms. If the performance of this is not crucial (I suspect this is<br class="">the case), then it should not harm to take the normal entry through JNI<br class="">for generic arraycopy using C1.<br class=""><br class="">But I think trying to get rid of that path could be a follow-up RFE<br class="">instead.<br class=""><br class="">Looks good.<br class=""><br class="">Thanks,<br class="">/Erik<br class=""><br class="">On 2018-03-13 11:02, Roman Kennke wrote:<br class=""><blockquote type="cite" class="">Hi Erik,<br class=""><br class="">thanks for doing this! It looks good to me!<br class=""><br class="">I'm putting this up here again, with commit msg fixed:<br class=""><a href="http://cr.openjdk.java.net/~rkennke/8198445/webrev.03/" class="">http://cr.openjdk.java.net/~rkennke/8198445/webrev.03/</a><br class=""><br class="">Can I get (final?) reviews? And then I'd put it through the new submit<br class="">repo and push it?<br class=""><br class="">Thanks, Roman<br class=""><br class=""><br class=""><blockquote type="cite" class="">Hi Roman,<br class=""><br class="">I would love to avoid using the switch to enumerate all different Java<br class="">types you could possibly have.<br class="">How about this? (was easier to cook something together than explain in<br class="">detail what I had in mind)<br class=""><br class="">Webrev:<br class="">http://cr.openjdk.java.net/~eosterlund/8198445/webrev.00/<br class=""><br class="">The idea is to let Access recognize arraycopy with element type "void",<br class="">and then perform a conservative byte arraycopy, that when<br class="">ARRAYCOPY_ATOMIC uses the natural atomicity of the element alignment by<br class="">using Copy::conjoint_memory_atomic().<br class=""><br class="">Thanks,<br class="">/Erik<br class=""><br class="">On 2018-03-08 21:12, Roman Kennke wrote:<br class=""><blockquote type="cite" class="">Am 08.03.2018 um 14:13 schrieb Erik Österlund:<br class=""><blockquote type="cite" class="">Hi Roman,<br class=""><br class="">On 2018-03-08 11:25, Roman Kennke wrote:<br class=""><blockquote type="cite" class="">Wouldn't that lead to mis-aligned accesses when you try to copy an<br class="">array<br class="">of, e.g., bytes, but shoehorn it to pointer-size? Or am I<br class="">misunderstanding?<br class=""></blockquote>No it would not. The element type is void, not void*. Because the<br class="">element type is type erased. And that would map to<br class="">Copy::conjoint_memory_atomic, which is precisely what<br class="">typeArrayKlass.cpp<br class="">maps such an arraycopy to today. Assuming elements are aligned to<br class="">their<br class="">natural size (which they are), Copy::conjoint_memory_atomic is<br class="">guaranteed to be at least as atomic as you need your elements to<br class="">be, up<br class="">to an element size of 8.<br class=""></blockquote>Ah, ok. Alright, so here comes rev.02:<br class=""><br class="">Differential:<br class="">http://cr.openjdk.java.net/~rkennke/8198445/webrev.02.diff/<br class="">Full:<br class="">http://cr.openjdk.java.net/~rkennke/8198445/webrev.02/<br class=""><br class="">Still passes tier1 tests.<br class=""><br class="">Ok now?<br class=""><br class="">Cheers, Roman<br class=""><br class=""><br class=""><blockquote type="cite" class=""><blockquote type="cite" class="">An idea would be to copy equal-sized elements using the corresponding<br class="">Copy::conjoint_XXX_atomic() calls, e.g. char and short would use the<br class="">short version, boolean and byte would use the byte version (or is<br class="">boolean sometimes int?!), float and int would use the int version and<br class="">long and double the long version. WDYT?<br class=""></blockquote>I'm not sure I see what the advantage of that would be.<br class=""><br class="">Thanks,<br class="">/Erik<br class=""><br class=""><blockquote type="cite" class="">Roman<br class=""><br class=""><br class=""><blockquote type="cite" class="">My spontaneous idea was to have an overload for type erased void*<br class="">that<br class="">when called with ARRAYCOPY_ATOMIC maps to<br class="">Copy::conjoint_memory_atomic<br class="">which AFAIK is conservatively atomic regardless of what the precise<br class="">element type is.<br class=""><br class="">But if you have better ideas, I am open for suggestions.<br class=""><br class="">Thanks,<br class="">/Erik<br class=""><br class="">On 2018-03-06 17:03, Roman Kennke wrote:<br class=""><blockquote type="cite" class="">Am 06.03.2018 um 14:35 schrieb Roman Kennke:<br class=""><blockquote type="cite" class="">Makes me wonder: why attempt to be smart in c1_Runtime1.cpp, when<br class="">we can<br class="">just as well call typeArrayOop::copy_array() and have it do the<br class="">right<br class="">thing? Or go even further and also do it for oop-arraycopy?<br class=""></blockquote>Something like:<br class=""><br class="">http://cr.openjdk.java.net/~rkennke/8198445/8198445-1.patch<br class=""><br class="">This wouldn't compile because of bunch of missing<br class="">arraycopy_conjoint_atomic defintions for extra types like jfloat,<br class="">jdouble, jboolean, etc, which in turn would be missing the same<br class="">Copy::conjoint_jFluffs_atomic() which drags in a bunch of platform<br class="">specific stuff... and my question before I go there is: do we want<br class="">all<br class="">that? Or can you think of a better way to solve it?<br class=""><br class="">Roman<br class=""><br class=""><blockquote type="cite" class="">Roman<br class=""><br class=""><blockquote type="cite" class="">The ARRAYCOPY_ATOMIC decorator makes the arraycopy atomic over<br class="">the<br class="">size<br class="">of the passed in elements.<br class="">In this case, it looks like the address has been type erased to<br class="">void*,<br class="">and hence lost what the element size was. There is currently no<br class="">overload<br class="">accepted for type erased element - only accurate elements {jbyte,<br class="">jshort, jint, jlong}.<br class=""><br class="">So it looks like an overload must be added to accept type erased<br class="">void*<br class="">elements and make that call conjoint_memory_atomic when the<br class="">ARRAYCOPY_ATOMIC decorator is passed in.<br class=""><br class="">Thanks,<br class="">/Erik<br class=""><br class="">On 2018-03-06 13:56, David Holmes wrote:<br class=""><blockquote type="cite" class="">On 6/03/2018 10:54 PM, Erik Österlund wrote:<br class=""><blockquote type="cite" class="">Hi David,<br class=""><br class="">It is atomic with the ARRAYCOPY_ATOMIC decorator. If that<br class="">comment is<br class="">correct (I do not know if it is), then the ARRAYCOPY_ATOMIC<br class="">decorator<br class="">should probably be used here.<br class=""></blockquote>If that code implements a Java array copy then yes it is<br class="">required to<br class="">be 32-bit atomic. Do you need the decorator to get 32-bit<br class="">atomicity?<br class=""><br class="">David<br class=""><br class=""><blockquote type="cite" class="">Thanks,<br class="">/Erik<br class=""><br class="">On 2018-03-06 13:48, David Holmes wrote:<br class=""><blockquote type="cite" class="">Hi Roman,<br class=""><br class="">Not a review as I'm not familiar enough with the Access API,<br class="">but in<br class="">src/hotspot/share/c1/c1_Runtime1.cpp the comments above the<br class="">changed<br class="">code need updating - probably deleting. I assume the Access<br class="">API<br class="">arraycopy is atomic?<br class=""><br class="">Thanks,<br class="">David<br class=""><br class="">On 6/03/2018 9:56 PM, Roman Kennke wrote:<br class=""><blockquote type="cite" class="">Currently, the Access API is only used for oop-arraycopy, but<br class="">not for<br class="">primitive arraycopy. GCs might want to intercept this too,<br class="">e.g.<br class="">resolve<br class="">src and dst arrays.<br class=""><br class="">There *is* an implementation of primitive arraycopy in the<br class="">Access API,<br class="">but it doesn't even compile, because Raw::arraycopy() does<br class="">not<br class="">take<br class="">src<br class="">and dst oop operands, but it's called like that. The only<br class="">reason<br class="">why<br class="">this does not blow up (I think) is that because nobody calls<br class="">it,<br class="">the<br class="">compiler doesn't even get there.<br class=""><br class="">This change fixes the Access API/impl and adds the relevant<br class="">calls into<br class="">it (in C1 and runtime land). C2 uses arraycopy stubs (which<br class="">cannot be<br class="">handled here) or calls out to the ArrayKlass::copy_array(),<br class="">which<br class="">should<br class="">be covered with this change.<br class=""><br class="">It should be possible to use the same Access API for<br class="">Java-array <-><br class="">native-array bulk transfers, which currently use the rather<br class="">ugly<br class="">typeArrayOop::XYZ_addr() + memcpy() pattern. I'll address<br class="">that<br class="">in a<br class="">separate change though.<br class=""><br class="">http://cr.openjdk.java.net/~rkennke/8198445/webrev.00/<br class=""><br class="">Tests: tier1 ok<br class=""><br class="">Please review!<br class="">Thanks, Roman<br class=""><br class=""></blockquote></blockquote></blockquote></blockquote></blockquote></blockquote></blockquote></blockquote></blockquote></blockquote></blockquote></blockquote><br class=""></blockquote><br class=""></blockquote><br class=""><br class=""></blockquote><br class=""><br class=""></div></div></blockquote></div><br class=""></div></body></html>