RFR: 8261941: Use ClassLoader for unregistered classes during -Xshare:dump

[Calvin Cheung]

On Fri, 10 Sep 2021 22:26:20 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> I think we should try to decouple the loading of unregistered classes from ClassLoader/ClassloaderExt. Maybe these new functions can be placed in a new file, shared/cds/unregisteredClasses.cpp, and the API entry point would be `UnregisteredClasses::load_class(name, path)`.

I've moved the new code to unregistedClasses.[c|h]pp.
> 
> Also, `ClassLoaderExt::find_classpath_entry_from_cache` shouldn't be needed anymore. If the JAR file doesn't exist, URLClassLoader will throw an exception, so we don't need to create a ClassPathEntry just to check for the JAR file's existence.

As we've discussed off-line, after removing the `ClassLoaderExt::find_classpath_entry_from_cache`, the calculation of the length and crc of the ClassFileSteam will be performed in `jvm_define_class_common()`.
> 
> I wonder if there's any code in classLoader.cpp that was used only for supporting the loading of unregistered classes. If so, it can be removed, too.

I don't see any functions in classLoader.cpp could be removed. However, I think the `ClassLoader::record_result()` could be simplified since it is no longer called with unregistered classes. I'd prefer handle the simplification in a follow-up bug.

> src/hotspot/share/classfile/classLoaderExt.cpp line 299:
> 
>> 297: class URLClassLoaderTable : public ResourceHashtable<
>> 298:   Symbol*, Handle,
>> 299:   7, // prime number
> 
> I think 7 is too small. Maybe 137?

Ok. I've increased it to 137.

> src/hotspot/share/classfile/classLoaderExt.cpp line 327:
> 
>> 325: Handle ClassLoaderExt::create_and_add_url_classloader(Symbol* path, TRAPS) {
>> 326:   Handle url_classloader = create_url_classloader(path, CHECK_NH);
>> 327:   bool added = _url_classloader_table->put(path, url_classloader);
> 
> added is not used. Did you want to assert that `added == true`?
> 
> I think it's better to move the body of this function to line 342. Then it would be clear that the entry doesn't exist (and there's no need to check for `added`).

Modified per your suggestions and I don't need to keep the bool variable.

> src/hotspot/share/classfile/classLoaderExt.cpp line 336:
> 
>> 334:     _url_classloader_table = new (ResourceObj::C_HEAP, mtClass)URLClassLoaderTable();
>> 335:     Handle url_classloader = create_and_add_url_classloader(path, CHECK_NH);
>> 336:     return url_classloader;
> 
> I think the above 2 lines should be removed and the creation should be done in a single place. This means `_url_classloader_table->get()` would be unnecessarily called when the table is created, but I think that's OK since this happens only once.

Done.

> src/hotspot/share/classfile/classLoaderExt.cpp line 368:
> 
>> 366:   assert(result.get_type() == T_OBJECT, "just checking");
>> 367:   oop obj = result.get_oop();
>> 368:   InstanceKlass* k = InstanceKlass::cast(java_lang_Class::as_Klass(obj));
> 
> No need for the temp variable `k` since it's immediately returned.

Fixed.

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

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