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

[Thomas Stuefe]

On Tue, 30 Mar 2021 22:46:30 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>>> I deleted this branch by mistake, now restored.
>>> 
>>> > I'm not sure this is correct. Your new non-TRAPS Metaspace::allocate() would fail every time the GC threshold is touched. Where the old TRAPS version would break through the threshold and allocate successfully.
>>> 
>>> I realize this. It's just an attempt to allocate and it's designed to be used during a safepoint for only this allocation. I could change this to only call the non-TRAPS version of MethodCounters if we're at a safepoint? Would that help? Then the only time we'll miss out on metaspace counters periodically is if they were created to set breakpoints in a safepoint.
>> 
>> I think that would be better. I am unclear on what happened in this case before; did we also miss out on allocating the Counters?
>> 
>>> 
>>> I'd hate for this special case to know more about metaspace, ala calling ClassLoaderMetaspace::expand_and_allocate.
>> 
>> Even within Metaspace::allocate(no TRAPS)? Its in metaspace land, surely it would be fine to call expand there like this:
>> 
>> MetaWord* Metaspace::allocate(ClassLoaderData* loader_data, size_t word_size, MetaspaceObj::Type type) {
>>   MetaWord* result = loader_data->metaspace_non_null()->allocate(...);
>>   if (!result) {
>>     MetaWord* result = loader_data->metaspace_non_null()->expand_and_allocate(...);
>>   }
>> 
>> (Note that I will be gone into vacation shortly and I'm a bit short on time; I'm not sure I can finish this review. If you go with your approach, my only request would be to comment the prototypes for the two allocate functions a bit clearer and/or maybe rename one as allocate_no_exception or the other as allocate_with_exception)
>
>> I think that would be better. I am unclear on what happened in this case before; did we also miss out on allocating the Counters?
> 
> Before this change, it was very unlikely that allocating metaspace counters in a breakpoint safepoint ran out of memory, so never threw the exception.  Or else they did and returned NULL and all of the code around their allocation has handling for a null return.  We're working on enforcing what should be a rule that only JavaThreads can throw exceptions and this was an exception to that. :)
> 
>> Re: expand_and_allocate()
> 
> I didn't want to expose internal metaspace functions or more handling for this special case, and that would prevent the nice sharing of most of the Metaspace::allocate code. It's allowed for method counters to return NULL here.  If it's not long term, we should move the breakpoint counters to Method (but it would increase Method by a pointer size, which isn't good).
> 
> I will rename the functions as requested.  Have a nice vacation and thank you for your comments.

Thanks for explaining! I am fine with the change in this case.

-------------

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