RFR: 8251850: Simplify ResourceMark constructors using delegation
Ioi Lam
ioi.lam at oracle.com
Sun Aug 16 05:25:30 UTC 2020
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.
>
More information about the hotspot-runtime-dev
mailing list