RFR: 8251850: Simplify ResourceMark constructors using delegation

[Thomas Stüfe]

On Sat, Aug 15, 2020 at 2:43 PM Kim Barrett <kim.barrett at oracle.com> wrote:

> > 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 agree that constness is underused in hotspot code. I use it a lot, so the
"almost never" part is not true, but some developers don't. I disagree
about the wider community. And neither is really a good argument - if it is
useful, we should use it.

Constness is a very useful instrument of the language. It clearly
communicates intent and prevents accidents. Why would we deny ourselves
such a tool?

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

Maybe I miscommunicated. RM is a bookmark into an Arena; a savepoint, a
frozen snapshot. Once taken it must never change because that would be
wrong. This is a good example for an immutable object. So why not express
that in code?

As it is, one has to read the whole code to find out that none of the
members are ever modified after construction. But one glance at a "const"
member would make this immediately clear, and would also show that this is
no accident but a deliberate design decision.

So, the frozen state is expressed by
(_area+_chunk+_hwm+_max+_size_in_bytes) (bit excessive, but that is another
topic). None of those must be  changed after initialization so all of them
should be const. For the pointers, I mean the pointer value to be const of
course.

As for pointer destinations: without letting changes ripple out too much,
at least _previous_resource_mark could be both const and destination const
(should be made so in Thread as well of course) since no-one will ever
change that mark if it is immutable.

_area cannot be destination-const since we use it to modify the internal
members of the arena when resetting (which is a bit eww but maybe it is
just me). Same goes for _chunk. _hwm and _max could in theory be
destination const, when done in a wider cleanup.

---

But seriously, if I were to redesign Arena and ResouceMark, I would define
some opaque public type in arena as state, and let ResourceMark store a
const version of that state, and hand it back to the Arena for it to reset
itself, like this:

Arena {
public:
 struct state_t { ... hwm, max, whatever };
 state_t get_state() const;
 void reset_to_state(const state_t& state);
}

ResourceMark {
 Arena* const _arena;
 const state_t _state;
public:
  ResourceMark(Arena* a) _state(a->get_state()) {}
  void reset() const { _arena->reset_to_state(_state); }
}

That way RM can be simple and dumb and we don't need to expose any RA
innards to the outside world.

(if returning state by value is not to your taste we could have a "void
Arena::fill_state(state_t&) const" member, and would not be able to make
the state const in RM).


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

I remember staring at this code in frustration a long time ago - before
OpenJDK, when there was no community to ask, and no way to change things
upstream. I would not be surprised if no one at Oracle remembers :)


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

Okay, no strong emotions either, please do what you think makes sense.


> > 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.
>
>
I see. Thank you for the explanation.

Now that I read this I remember I ran into this myself at least once:
https://bugs.openjdk.java.net/browse/JDK-8183228 and seems I promptly
forgot. This is a sign that things are too complicated :) The issue seems
stalled, let's hope it moves forward.

Cheers, Thomas