Please review (S): fix placeholder table entry cleanup

[David Holmes]

Actually thinking more about this I'm unsure of this fix. I understand 
it will now clean up in a case where previously it didn't. But isn't 
there also a risk that the placeholder entry will be removed prematurely 
by a thread encountering a stackoverflow?

David

On 12/10/2012 5:51 PM, David Holmes wrote:
> Hi Karen,
>
> On 12/10/2012 4:04 AM, Karen Kinnear wrote:
>>
>> Fix to clean up placeholdertable entries in stackoverflow conditions.
>>
>> webrev: http://cr.openjdk.java.net/~acorn/7199207/webrev
>
> So the basic problem is that the thread nominally responsible for
> cleaning up the placeholder doesn't because a StackOverflow causes the
> call to resolve_or_fail to issue a return (via the CHECK_ macro) that
> skips the cleanup - right?
>
> So here:
>
> 1263 resolve_super_or_fail(class_name, name, class_loader, Handle(),
> false, CHECK_(nh));
> 1264 }
> 1265 // Caller's of resolve_super_or_fail need to clean up child's
> placeholder entry
> 1266 // Because define class path needs to track entry until
> systemDictionary entry created
>
> don't we need to change the CHECK_(nh) to just THREAD as is done in
> handle_parallel_super_load() ?
>
> Also if we're running out of stack do we know that we have enough stack
> to perform this cleanup work? If we don't will we abort instead?
>
> Looking at the miscellaneous comment updates I notice that here:
>
> 442 / 867/ 915 // SystemDictionary::do_unloading() asserts that classes
> in the SD are only
>
> we make a claim about an assertion, but in the actual code for
> SystemDictionary::do_unloading there is no direct assertion (I know you
> only added the "in the SD" part :) )
>
> 1790 // Assumes classes in the SystemDictionary are only unloaded at a
> safepoint
> 1791 // Note: anonymous classes are not in the SD
>
> Is it an assumption or requirement? Should 1791 add "and so are not
> unloaded by this method" ? Do anonymous classes deserve some more
> in-depth commentary somewhere in here?
>
> Cheers,
> David
>
>> bug link: https://jbs.oracle.com/bugs/browse/JDK-7199207
>>
>> Thanks to Stefan Karlsson for identifying the problem and
>> the initial fix.
>>
>> Tests run:
>> Originally reported chain007 with ThreadStackSize=128 and
>> original options.
>>
>> vm.quick.testlist
>> vm.parallel_class_loading.testlist
>> jck: non-interactive jck, api, lang
>> jtreg: java.lang.instrument
>> RunThese full
>> ctw nightly.testlist
>>
>> thanks,
>> Karen
>>
>>