RFR: 8251850: Simplify ResourceMark constructors using delegation
Kim Barrett
kim.barrett at oracle.com
Sat Aug 15 11:42:30 UTC 2020
> 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.
More information about the hotspot-runtime-dev
mailing list