RFR: 8251850: Simplify ResourceMark constructors using delegation

[David Holmes]

Hi Kim,

Not a full review - sorry. Have you tested what happens if a resource 
leak is introduced? The _warned variable was used, IIUC, to avoid 
hitting recursive errors during error reporting.

Having flagged that I may not be around to see your response as I'm on 
vacation after today - sorry.

Cheers,
David

On 17/08/2020 12:24 pm, Kim Barrett wrote:
>> On Aug 15, 2020, at 8:31 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>
>>> On Aug 14, 2020, at 10:29 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>>
>>> Please review this simplification of the constructors for ResourceMark,
>>> accomplished using delegating constructors.
>>>
>>> Also made the constructors "explicit" and the class non-copyable.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8251850
>>>
>>> Webrev:
>>> https://cr.openjdk.java.net/~kbarrett/8251850/open.00/
>>>
>>> Testing:
>>> mach5 tier1
>>
>> I should have looked further along in resourceArea.hpp. All the
>> changes I made to ResourceMark? DeoptResourceMark should have similar
>> changes made to it.
>>
>> Well, I have a first round of comments from the ResourceMark changes.  I can
>> take those into account for DeoptResourceMark.
> 
> This ended up engendering a fairly substantial revision.  I've taken at
> least some of Thomas's comments into account along the way.
> 
> Changed ResourceMark and DeoptResourceMark to share ResourceMarkImpl, which
> provides most of the implementation of both, eliminating the copy-paste-modify
> "reuse" that used to be there.
> 
> Moved all of the state capture and rollback support into ResourceArea. The
> mark objects include a captured state object and pass that back to the
> ResourceArea when doing rollbacks.
> 
> Cleaned up the _nesting checking.
> 
> Removed nonsensical ResourceArea::_warned.
> 
> Maybe I should change the bug summary from
> "Simplify ResourceMark constructors using delegation"
> to something like
> "Refactor ResourceMark and DeoptResourceMark for better code sharing"
> 
> Note: ResourceArea's handling of UseMallocOnly probably doesn't work in an
> optimize build. ("probably" because I haven't actually tested it.) The
> allocation side of it is #ifdef ASSERT, because functions it calls from
> Arena are debug-only. But the free side is not conditionalized, so could be
> used in any non-product build when the option is enabled. But maybe it's not
> really a problem because the free side won't find any work. But this
> conditionalization inconsistency is odd.
> 
> New webrevs:
> full: https://cr.openjdk.java.net/~kbarrett/8251850/open.01/
> incr: https://cr.openjdk.java.net/~kbarrett/8251850/open.01.inc/
> 
> Testing:
> mach5 tier1-3
> local hotspot:tier1 with -XX:+UseMallocOnly
> local hotspot:tier1 with -XX:+ZapResourceArea
> 
> Note that compiler/loopopts/TestDeepGraphVerifyIterativeGVN.java times out
> when run with -XX:+UseMallocOnly, with or without these changes.
> 
>