RFR: 8242452: During module definition, move conversion of packages from native to VM

[Claes Redestad]

Hi Ioi,

On 2020-04-16 01:04, Ioi Lam wrote:
> Hi Claes,
> 
> This is very good optimization.

Thanks!

> I have a few nit picks:

Uh oh!

> 
> [1] I think the following is unnecessary. The original 
> Java_java_lang_Module_defineModule0 and GetInternalPackageName function 
> did not perform null check or type check. I think this should be changed 
> to an assert.
> 
> The array comes from a Set<String>::toArray() in Module.java, so each 
> array element must be a String and the array has no NULLs in it.
> 
>      oop pkg_str = packages_h->obj_at(x);
>      if (pkg_str == NULL || pkg_str->klass() != 
> SystemDictionary::String_klass()) {
>        THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
>                  err_msg("Bad package name"));
>      }

I think if we are to loosen the checking done in the VM endpoints - by
making them asserts or otherwise - then I think that should be a
separate change. The performance overhead is negligible either way (a
few thousand instructions).

> 
> The other 3 APIs in Module.c that you modified had a null check on the 
> string. I think this check should be preserved in Module.c (which has 
> more intimidate knowledge whether the string should be null or not). The 
> corresponding HotSpot functions should assert for NULL.
> 
> (With the current patch, for example, the null check on the path 
> Java_java_lang_Module_addExports0 -> JVM_AddModuleExports -> 
> Modules::add_module_exports_qualified was unintentionally removed).

There's a null check of packages in Modules::add_modules_exports. I
could add a redundant one in add_module_exports_qualified to make it
more clear - but if there's a missing check in any path then that's
unintentional and a bug (also test bug, since tests for these
check that sending in NULL for package_name throws NPE)

> 
> 
> [2] This code is repeated twice. The duplication was there before your 
> change, but now that you're changing it, I think we should consolidate 
> the code as well.
> 
>    int version_len = 0;
>    const char* module_version = get_module_version(version, version_len);
>    TempNewSymbol version_symbol;
>    if (module_version != NULL) {
>      version_symbol = SymbolTable::new_symbol(module_version, version_len);
>    } else {
>      version_symbol = NULL;
>    }

I've cleaned this up.

> 
> 
> [3] In Modules::add_module_exports, the package_h doesn't seem 
> necessary, as it's immediately converted back to an oop to pass to 
> as_internal_package().
> 
>    Handle package_h(THREAD, JNIHandles::resolve_non_null(package_name));
>    const char* pkg = as_internal_package(package_h(), buf, sizeof(buf), 
> len);

Fixed.

> 
> [4] Why is the change in Modules::get_named_module necessary? The caller 
> already has a const char* for the package name, and it seems like the 
> new code will covert that to a String, and then covert it back to a 
> const char*. The new code also does "." -> "/" conversion, which wasn't 
> there before. So it seems like this is not necessary and would 
> potentially cause behavior differences.
> 
> jobject Modules::get_named_module(Handle h_loader, Handle 
> h_package_name, TRAPS) {
>    ....
>    const char* pkg = as_internal_package(h_package_name(), buf, 
> sizeof(buf), pkg_len);

Fixed (reverted)

> 
> [5] It's better to avoid unprotected oops when you are creating a handle:
> 
>    objArrayOop packages_oop = objArrayOop(JNIHandles::resolve(packages));
>    objArrayHandle packages_h(THREAD, packages_oop);
> 
> ->
> 
>    objArrayHandle packages_h(THREAD, 
> objArrayOop(JNIHandles::resolve(packages)));
> 
> The code works fine today, but in the future someone may unintentionally 
> use packages_oop across a potential safe point. It's better to prevent 
> that from happening if you can.

Fixed.

New webrev: http://cr.openjdk.java.net/~redestad/8242452/open.01/

Thanks
/Claes