Harold, Looks good. Many thanks!
Karen > On Feb 12, 2018, at 3:50 PM, harold seigel <harold.sei...@oracle.com> wrote: > > Hi Karen, > > Thanks for looking at this! > > I re-ran the ParallelClassLoading tests with no options, with > -XX:+AllowParallelDefineClass, and with -XX:+AlwaysLockClassLoader, and all > the tests passed. I also determined that the few Mach5 regression test > failures that I encountered were unrelated to my change. > Please see this latest webrev that adds 12 as an expiration date, removes the > 'guarantee' section, and fixes the comment at line 786 of > systemDictionary.cpp. > http://cr.openjdk.java.net/~hseigel/bug_8184289.3/webrev/index.html > <http://cr.openjdk.java.net/~hseigel/bug_8184289.3/webrev/index.html> > Thanks, Harold > > On 2/12/2018 2:39 PM, Karen Kinnear wrote: >> Harold, >> >> Thanks for doing this. >> >> I think you told me that >> 1) the version change has made it in >> 2) you also put 12 as an expiration date >> 3) you are running the ParallelClassLoading tests with the remaining two >> flags (you’ve already >> run them without any flags): >> AllowParallelDefineClass = true and AlwaysLockClassLoader=true >> >> In terms of the guarantee in question >> // For UnsyncloadClass only >> 848 // If they got a linkageError, check if a parallel class load >> succeeded. >> 849 // If it did, then for bytecode resolution the specification >> requires >> 850 // that we return the same result we did for the other thread, >> i.e. the >> 851 // successfully loaded InstanceKlass >> 852 // Should not get here for classloaders that support parallelism >> 853 // with the new cleaner mechanism, even with >> AllowParallelDefineClass >> 854 // Bootstrap goes through here to allow for an extra guarantee >> check >> 855 if (UnsyncloadClass || (class_loader.is_null())) { >> 856 if (k == NULL && HAS_PENDING_EXCEPTION >> 857 && >> PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) { >> 858 MutexLocker mu(SystemDictionary_lock, THREAD); >> 859 InstanceKlass* check = find_class(d_hash, name, dictionary); >> 860 if (check != NULL) { >> 861 // Klass is already loaded, so just use it >> 862 k = check; >> 863 CLEAR_PENDING_EXCEPTION; >> 864 guarantee((!class_loader.is_null()), "dup definition for >> bootstrap loader?"); >> 865 } >> 866 } >> 867 } >> >> 1) I agree you can remove the entire section >> - the guarantee was there for future proofing in case we ever allowed >> parallel class loading of the >> same class for the null loader and to make sure I didn’t have any >> logic holes. >> - I would not put an assertion for the first half of the condition - I >> would remove completely >> - the code currently prevents parallel class loading of the same class >> for the null loader at: >> resolve_instance_class_or_null see line 785 … >> while (!class_has_been_loaded && old probe && old >> probe->instance_load_in_progress()) { >> // case x: bootstrap class loader: prevent futile class loading, >> // wait on first requestor >> if (class_loader.is_null()) { >> SystemDictionary_lock->wait(); >> >> This logic means that there is a registered INSTANCE_LOAD on this >> placeholder entry. >> >> >> Other minor comments (sorry if you already got these and I missed them in >> earlier emails) >> - all in SystemDictionary.cpp >> >> 1. line 72 comment “Five cases:” -> “Four cases:” >> So you removed case 3 and renumbered, so old references to case 4 -> case 3 >> ,and old references >> to case 5 become case 4: >> >> So - line 786, “Case 4” -> “case 3” >> >> thanks, >> Karen >> >> >>> On Feb 12, 2018, at 11:13 AM, harold seigel <harold.sei...@oracle.com >>> <mailto:harold.sei...@oracle.com>> wrote: >>> >>> Hi Alan, >>> >>> Thanks for looking at this. >>> >>> Harold >>> >>> On 2/12/2018 2:52 AM, Alan Bateman wrote: >>>> On 12/02/2018 06:54, David Holmes wrote: >>>>> Hi Harold, >>>>> >>>>> Adding core-libs-dev so they are aware of the ClassLoader change. >>>> Thanks, that part is okay and good to see this going away. >>>> >>>> -Alan >>> >> >
