RFR: 8251850: Simplify ResourceMark constructors using delegation

[Ioi Lam]

On 8/15/20 4:42 AM, Kim Barrett wrote:
>> 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.

Hi Kim,

Thanks for the explanation. Since the new function 
current_thread_or_null() is used in only one place, I think it's better 
to get rid of it, and change this constructor as:

   explicit ResourceMark(ResourceArea* area)
     : ResourceMark(area, DEBUG_ONLY(Thread::current_or_null()) 
NOT_DEBUG(nullptr)) {}

Thanks
- Ioi

PS, NOT_PRODUCT_ARG does come with a comma:

#define NOT_PRODUCT_ARG(arg) arg,





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