RFR: 8251850: Simplify ResourceMark constructors using delegation

[Kim Barrett]

> On Aug 17, 2020, at 3:31 AM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> 
> 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.

Thanks.  I've fixed the typo locally.

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

I prefer having the DEBUG_ONLY close to the code that cares, i.e.
ResourceArea has DEBUG_ONLY _nested, and ResourceArea should be responsible
for dealing with that.  That these functions end up being nops in release
builds is an implementation detail of ResourceArea.

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

We could, but that didn’t seem important.

> typo
> 
> // Roll back the allocation state to the indicates state values.
> 
> s/indicates/indicated

Fixed locally.

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

I was surprised by this too. I spent a little bit of time trying to figure
out why, and whether those uses could instead just use several scoped
ResourceMarks, but eventually decided I just wasn't sufficiently familiar
with that code, and this change had already gotten a bit larger than I'd
been expecting.

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

I don't think the extra firing is really a problem, though presumably the
calling code could be changed to eliminate an unnecessary reset that shortly
preceeds the destructor.  OTOH being explicit about where the reset is safe
might make future modifications of those callers a little easier.

> 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 don't know if that distinction would really carry its weight. And I think
it would be better to first see if the manual cases can be eliminated.

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

Yeah, someone should probably do that. :)

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

Yeah, you are right that HandleMark and ResourceMark are very similar, and
probably similarly mergeable.  I think I will leave that to someone else though.