RFR:7199207 Placeholders cleanup

[Karen Kinnear]

Thanks very much David.

thanks,
Karen

On Jan 10, 2013, at 5:32 PM, David Holmes wrote:

> Looks good.
> 
> Thanks Karen.
> 
> David
> 
> On 11/01/2013 2:34 AM, Karen Kinnear wrote:
>> Updated webrev from both David and Coleen's suggestions (many thanks)
>> 
>> http://cr.openjdk.java.net/~acorn/7199207.postreview/webrev
>> 
>> Rerunning parallel_class_loading.testlist (which includes circularity testing) and sysdict.
>> 
>> thanks,
>> Karen
>> 
>> On Jan 10, 2013, at 10:52 AM, Karen Kinnear wrote:
>> 
>>> David,
>>> 
>>> Good question. I will add an assertion !HAS_PENDING_EXCEPTION before throwing the
>>> circularity error. And I moved it to right after releasing the lock, which is back to where it
>>> used to be, so I believe the only risk of a PENDING_EXCEPTION is if you called
>>> resolve_instance_class_or_null with one already.
>>> 
>>> So can I count you as a code reviewer?
>>> 
>>> thanks,
>>> Karen
>>> 
>>> On Jan 9, 2013, at 5:42 AM, David Holmes wrote:
>>> 
>>>> Hi Karen,
>>>> 
>>>> Can't really comment on the validity or otherwise of the approach but one query:
>>>> 
>>>> 811   // must throw error outside of owning lock
>>>> 812   if (throw_circularity_error) {
>>>> 813     ResourceMark rm(THREAD);
>>>> 814     THROW_MSG_0(vmSymbols::java_lang_ClassCircularityError(), name->as_C_string());
>>>> 815   }
>>>> 816
>>>> 817   if (HAS_PENDING_EXCEPTION || k.is_null()) {
>>>> 818     return NULL;
>>>> 819   }
>>>> 
>>>> Is it the case that we can not have an exception pending if we need to throw the circularity error? If so should we should assert that. Otherwise are we concerned about the exception we will lose?
>>>> 
>>>> David
>>>> 
>>>> On 5/01/2013 6:54 AM, Karen Kinnear wrote:
>>>>> Please review:
>>>>> http://cr.openjdk.java.net/~acorn/7199207.fix2merge/webrev/
>>>>> 
>>>>> bug: NPG: Crash in PlaceholderTable::verify, after a StackOverflow during class loading
>>>>> https://jbs.oracle.com/bugs/browse/JDK-7199207
>>>>> 
>>>>> Thanks to Stefan Karlsson for the initial analysis and the test case.
>>>>> 
>>>>> Thanks to Coleen's suggestion, I've simplified the placeholder table entry cleanup.
>>>>> Specifically the placeholder table is not used to keep the instanceKlass alive for GC during class resolution, and no
>>>>> other logic depends on the contents of the placeholder table. Therefore rather than retaining the entries for the entire
>>>>> span of resolve_from , entries are explicitly added and removed for each specific resolution action: LOAD_INSTANCE,
>>>>> LOAD_SUPER and DEFINE_CLASS (used for parallel class loading).
>>>>> 
>>>>> Tests run:
>>>>> Original test case from bug (linux 32 bit client)
>>>>> regression tests on solaris/sparc 32 bit:
>>>>>  vm.sysdict.testlist
>>>>>  vm.quick.testlist
>>>>>  vm.parallel_class_loading.testlist: no flags, -XX:+AllowParallelDefineClass, -XX:+UnsyncLoadClass, -XX:+PrintSystemDictionaryAtExit
>>>>>  java.lang.instrument: jtreg
>>>>>  jli with -Xcomp
>>>>>  ctw nightly.testlist
>>>>> tests in progress on solaris/sparc 32 bit:
>>>>>  runThese -Xcomp
>>>>>  jck vm
>>>>> 
>>>>> thanks,
>>>>> Karen
>>>>> 
>>>>> 
>>> 
>>