RFR: 8251850: Simplify ResourceMark constructors using delegation

[Thomas Stüfe]

On Mon, Aug 17, 2020 at 3:15 PM Kim Barrett <kim.barrett at oracle.com> wrote:

> > 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.
>
>
All good then. To me this looks fine, don't need another webrev.

Thanks, Thomas