Please review (S): fix placeholder table entry cleanup

[Karen Kinnear]

David,

On Oct 12, 2012, at 3:51 AM, 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() ?
Yes - good catch. I will update the webrev. Thank you.
> 
> 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?
This fixes one stackoverflow problem.
Yes - I can also run into a stack overflow in the concurrent library which runs into other issues that are still open.
Yes - if I put in too much debug printing I can run into a stack overflow in the print library.
Yes - we do not know that we have enough stack to handle all cases, we do however know that the depth of the clean up is
less than the usual depth of the actual resolve_super_or_fail which of course can call out to the java libraries, etc. etc.
at least in the handle_parallel_super_ case. The load_shared_class should not be calling out to the java libraries of course.
> 
> 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 :) )
I noticed that too.
> 
> 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?
We'll let the anonymous classes author pick up on that, since I don't personally have an in-depth enough understanding of how they are 
unloaded for instance.

Great comments, and many thanks,
Karen
> 
> 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
>> 
>>