Please review (S): fix placeholder table entry cleanup

[Karen Kinnear]

David,

That was exactly my initial concern when Stefan proposed the cleanup in handle_parallel_super...
Which is why I spent days pouring over the logic and assumptions again in great detail and I
will send you the details of my current understanding - but it will be later today since I need to
summarize it. It would be good to sanity check this with your understanding.

thanks,
Karen

On Oct 12, 2012, at 4:37 AM, David Holmes wrote:

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