RFR: 8251850: Simplify ResourceMark constructors using delegation

[Thomas Stüfe]

Hi Kim,

More remarks, but I slowly enter bikeshedding territory, please decide what
you take from this. Apart from the typo this patch looks already fine in
its current form.

---

activate_state, deactivate_state

Matter of taste, I would define those inside an ASSERT block and wrap calls
in DEBUG_ONLY, rather than have empty implementations in release code.

---

In theory one could make the _area pointer part of the state instead of
keeping it as an extra member of RM, since a state is bound to a specific
area. And we nowhere test that the area we rollback to is the original one
we took the state from. But it's not important.

---

typo

// Roll back the allocation state to the indicates state values.

s/indicates/indicated

---

I was surprised to find out that RMs can be triggered prematurely, and also
multiple times, by manually calling reset_to_mark(). It's done in two
places in the compiler, PhaseCFG::global_code_motion()) and
PhaseChaitin::Register_Allocate(). The latter case is the only one I found
which resets a mark multiple times.

In those two mentioned cases, the mark also fires one additional time when
the object goes out of scope, since the mark never gets disarmed. I guess
harmless, but surprising and at least unnecessary (in both cases the arena
goes out of scope right after).

One future improvement may be to split this functionality into two classes,
one which does auto resetting in destructor and does not expose
"reset_to_mark", one which exposes "reset_to_mark" and has an empty
destructor. "AutoResourceMark" vs "ManualResourceMark" maybe?

---

I did not find gtests for all that. Maybe we should write some.

---

Finally, I wonder whether we could just merge Arena and ResourceArea, for
simplicity sake. Move the ability to save/restore state up to Arena and get
rid of ResourceArea. There is HandleArena, derived from Arena, and
HandleMark, which looks similar to ResourceMark. They would also benefit
from inheriting that ability.


Thanks, Thomas



On Mon, Aug 17, 2020 at 4:24 AM Kim Barrett <kim.barrett at oracle.com> 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.
>
>
>