RFR: 8251850: Simplify ResourceMark constructors using delegation

[Kim Barrett]

> On Aug 14, 2020, at 11:27 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> Hi Kim,
> 
> This looks good overall.

Thanks.

> I found the following function a little confusing:
> 
>  108   static Thread* current_thread_or_null() {
>  109     return DEBUG_ONLY(Thread::current_or_null()) NOT_DEBUG(nullptr);
> 
> In usual HotSpot code, xxx_or_null() means "if there's no xxx, use". However, in this case, we do have a current thread, but we are just not using it in product build.

Maybe a better name would be something like "current_thread_for_debug"?
The point is we don't need the actual current thread at all in a
non-debug (not non-product, which also includes optimize) build, a
null pointer of the appropriate type will do.  And even in a debug
build the call might occur early, before threading has been set up,
so we might not have a current thread.

> I think it's clearer to changed the constructor
> 
>   88   ResourceMark(ResourceArea* area, Thread* thread) :
> 
> to
> 
>   88   ResourceMark(NOT_PRODUCT_ARG(Thread* thread) ResourceArea* area) :

That doesn't work.  (Besides missing a COMMA.)  That private
constructor needs to always have the Thread* argument in order to
distinguish it from the public (ResourceArea*) overload.

Also, PRODUCT is the wrong flag to be testing for any of this stuff.
All of the conditionalization here was, and should still be, based on
ASSERT, not on PRODUCT.

> and then use
> 
>  119   explicit ResourceMark(ResourceArea* area)
>  120     : ResourceMark(NOT_PRODUCT_ARG(Thread::current()) area) {}

That would be defining a constructor that delegates to itself in a
PRODUCT build. See above. (Besides also missing a COMMA here.)

> Also, these can be changed to NOT_PRODUCT_ARG -- but you need to move the corresponding field declarations up a few lines
> 
>   94     DEBUG_ONLY(COMMA _thread(thread))
>   95     DEBUG_ONLY(COMMA _previous_resource_mark(nullptr))

I don't think I understand what you were getting at here with the
"move the corresponding field declarations", but PRODUCT is the wrong
flag to be testing.