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> 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 >