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

[Ioi Lam]

On 4/16/20 12:56 PM, Claes Redestad wrote:
> 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)

Hi Claes,

Thanks for the explanation. I didn't notice that the NULL checks were 
moved down the call path but it looks good to me now.
>
>>
>>
>> [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)
>

Did you revert the change in jvmtiEnv.cpp as well? It's still changed in 
your latest webrev below but this change doesn't seem to be needed.

The rest of the changes look good to me. Thanks!

- Ioi


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