RFR: 8264149 BreakpointInfo::set allocates metaspace object in VM thread [v3]

[David Holmes]

On 31/03/2021 8:37 am, Coleen Phillimore wrote:
> On Tue, 30 Mar 2021 03:53:54 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>>>
>>>    Make which version of MethodCounters::allocate() is called clearer.
>>
>> src/hotspot/share/oops/method.cpp line 570:
>>
>>> 568:   if (current->is_Java_thread()) {
>>> 569:     Thread* THREAD = current;
>>> 570:     counters = MethodCounters::allocate(mh, THREAD);
>>
>> Can you add a comment before this line:
>>
>> // Use the TRAPS version for a JavaThread so it will adjust the GC  threshold if needed.
>>
>> Thanks.
> 
> ok, yes that says why.
> 
>> src/hotspot/share/oops/method.cpp line 572:
>>
>>> 570:     counters = MethodCounters::allocate(mh, THREAD);
>>> 571:     if (HAS_PENDING_EXCEPTION) {
>>> 572:       CLEAR_PENDING_EXCEPTION;  // MethodData above doesn't clear exception
>>
>> I don't understand the comment.
> 
> It's sort of a question, because the above code that does the same thing for MethodData doesn't clear the exception.  In this case we should clear the exception I believe.  I'll remove the comment.

Ah I see. All the callers of build_interpreter_method_data clear the 
exception themselves, rather than it being cleared internally. That 
seems odd.

>> src/hotspot/share/oops/method.cpp line 575:
>>
>>> 573:       CompileBroker::log_metaspace_failure();
>>> 574:       ClassLoaderDataGraph::set_metaspace_oom(true);
>>> 575:       return NULL;
>>
>> You could factor this out for both cases by testing "counters == NULL".
> 
> Yes, this is better.

Refactoring looks good!

Thanks,
David

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/3207
>