RFR: 8261480: MetaspaceShared::preload_and_dump should check exceptions [v2]

[David Holmes]

Hi Ioi,

All responses about future enhancements noted. A couple of specific 
follow ups ...

On 17/03/2021 11:06 am, Ioi Lam wrote:
> On Tue, 16 Mar 2021 04:09:40 GMT, David Holmes <dholmes at openjdk.org> wrote:
>> src/hotspot/share/classfile/classListParser.cpp line 527:
>>
>>> 525: }
>>> 526:
>>> 527: void ClassListParser::resolve_indy(Thread* current, Symbol* class_name_symbol) {
>>
>> Should current be a JavaThread here?
> 
> Currently all the `current` parameters will come from the THREAD/TRAPS of its caller, like
> 
> int ClassListParser::parse(TRAPS) {
>      ...
>      resolve_indy(THREAD, class_name_symbol);
> 
> I could change the caller to use THREAD->as_JavaThread(), but I think that's too much ceremony. It's better to change to `JavaThread*` after THREAD/TRAPS are changed to `JavaThread` (JDK-8252685)

Okay I will take care of that one.

>> src/hotspot/share/classfile/classLoaderExt.cpp line 288:
>>
>>> 286:                                           loader_data,
>>> 287:                                           cl_info,
>>> 288:                                           THREAD);
>>
>> I think I'd prefer to see a local introduced and then CHECK used rather than THREAD.
> 
> This is our perennial problem: CHECK cannot be used on a `return` statement :-(
> 
> See [JDK-8064811](https://bugs.openjdk.java.net/browse/JDK-8064811) (Use THREAD instead of CHECK_NULL in return statements)

Yes but my point is that rather than simply replace CHECK with THREAD I 
would prefer to see a local variable introduced so that we can use CHECK ie.

RetType ret = foo(CHECK);
return ret;

rather than:

return foo(THREAD);

>> src/hotspot/share/classfile/classLoaderExt.cpp line 298:
>>
>>> 296: static GrowableArray<CachedClassPathEntry>* cached_path_entries = NULL;
>>> 297:
>>> 298: ClassPathEntry* ClassLoaderExt::find_classpath_entry_from_cache(Thread* current, const char* path) {
>>
>> If this is executed by the vmThread at dump time then I'd prefer to  see current called vmThread.
> 
> This is executed only in the main Java thread.

Noted.

>> src/hotspot/share/classfile/classLoaderExt.cpp line 327:
>>
>>> 325:   Thread* THREAD = current; // For exception macros.
>>> 326:   new_entry = create_class_path_entry(path, &st, /*throw_exception=*/false,
>>> 327:                                       false, false, CATCH); // will never throw
>>
>> Given we are actually dealing with the VMThread which can't (or shouldn't!) actually throw exceptions I'm concerned about this chunk of code. I don't think we have a good idiom for these cases where the VMThread calls potentially exception throwing code in a way that should never throw an exception. Ideally all the exception-related fields would be in JavaThread.
> 
> All code changed by this patch executes in the main Java thread.

Noted. I will re-examine.

Thanks,
David

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