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

[Thomas Stuefe]

On Tue, 30 Mar 2021 02:38:44 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> src/hotspot/share/oops/method.cpp line 570:
>> 
>>> 568:   if (current->is_Java_thread()) {
>>> 569:     // For when TRAPS is JavaThread.
>>> 570:     counters = MethodCounters::allocate(mh, current->as_Java_thread());
>> 
>> I'm not at all clear what we are doing here and it seems premature to anticipate the TRAPS change to JavaThread. The code will need reworking for that change because you still check for a pending exception regardless of what type of Thread current is.
>> I don't see why we need to call a TRAPS version of allocate when we are going to clear any exception - just call the non-traps version (and remove the assert you added). Just because we are in a JavaThread it doesn't mean we have to throw exceptions.
>
> With this change, if the thread is not a Java thread, say the VMThread, we cannot throw an exception because we can't allocate a Java object for that exception. That's why we call the non-TRAPS version for !JavaThread.  So there's no need to check for a pending exception because it can't throw one. Also, eventually we want to enforce that only JavaThreads can throw/catch Java exceptions.
> The change requested by Thomas was that if we *can* thrown an exception, we should call the TRAPS version because that version will adjust the GC threshold.  We can't call GC with the non-TRAPS version.
> So this is the simplest thing.  We have two versions of this function (the TRAPS version calls the non-TRAPS version). The non-TRAPS version is the special one that can be used by the VMThread for allocating metadata, all other callers stay the same.
> I can remove the as_Java_thread() and line 569 and you can add it back when we're able to change TRAPS to JavaThread.

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

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

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