RFR: 8251850: Simplify ResourceMark constructors using delegation

[Kim Barrett]

> On Aug 15, 2020, at 3:01 AM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> 
> Hi Kim,
> 
> This is a good cleanup. I looked over the old version and there are a number of inconsistencies, most of which go away with this patch.

Thanks.

> RM can be made immutable now. All members can be made const (also _thread and _previous_resource_mark, if you reformulate the initializer a bit with a '?' expression). As well as all member functions. AFAICS none of the members get modified after construction, which makes sense considering what RM is.
> 
> Some of the pointer members could be made destination-const too, at least _previous_resource_mark. With RM being immutable there would be no reason ever to have a non-const RM*. I know constness can snowball a bit, but I think it's worth the work since it makes the code more clearer and prevents accidents.

That could be done, but it's stylistically almost never done in
HotSpot code, or even in the wider C++ community that I've ever seen.

I think changing the pointers (other than than _area) to pointer to
const isn't really an improvement, since what they are pointing at
isn't actually const, just happens to not be modified through that
specific copy of the pointer.

> In constructor:
> 
> +    _area->_nesting++;
> +    assert(_area->_nesting > 0, "must stack allocate RMs" );
> 
> Not your patch,but I do not understand this assert. 
> 
> Is it - as the comment says - to make sure RMs are allocated on the stack, as in StackObj? Would deriving from StackObj not be enough? How does this assert help then?
> 
> Or is it to ensure that all allocations happen under a RM? That should be checked in Arena::allocate(), and it is. Checking here is pointless since we just established an RM, so there is nothing to check.

I didn't try to figure out what that assert is checking. Looking at it
more carefully now, it might be doing a couple of things:

(1) Verifying that _nesting started from a good point, that it had
been properly initialized and there hadn't been some underflow
somewhere.

(2) Verifying that the increment of _nesting didn't overflow. But it
doesn't actually do that, since such overflow would be UB and the
compiler is free to treat it as impossible and delete the test.

The error message for the assert doesn't clarify things at all.

I think making any changes to that assert is a different problem than
what this change is doing.  I could file a new RFE to try to figure
out what to do about it.  (Certainly something should be done, since
I agree it's quite unclear what it's trying to verify.)

> small nit: I like that you made protected private, now you could just omit the qualifier altogether.

HotSpot code varies on that; some folks prefer the consistency of
always having it be explicit. I don't care that much either way.

> Since you are here ...
> 
> I'd prefer it if free_malloced_objects() would be DEBUG_ONLY() (so, not an empty prototype but completely removed) which would be consistent with other debug only code in this class. Side question, what is the difference between ASSERT and !PRODUCT?

Changing it to a DEBUG_ONLY declaration would be incorrect.  That
would require making the call DEBUG_ONLY too.  But the condition for
calling is UseMallcOnly, which is a develop flag, which is a PRODUCT
distinction, not a debug (ASSERT) distinction.

The difference between ASSERT and !PRODUCT is that the case of
(!PRODUCT && !ASSERT) is the so-called "optimized" build.  There are
periodic calls for eliminating this category of builds (JDK-8183287,
for example), and it often gets broken because most people don't use
or test it, but it has fans and so has never gone away.

The purpose of "optimized" builds (as explained to me by one of its
long-time proponents) is to have the performance characteristics of a
release build (so no extra checks that affect performance or timing),
but provide additional tools and data (printers, names, &etc) that we
want to exclude from a release build for reasons of saving space or
whatever. There's certainly lots of confusion around it though.