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

[Ioi Lam]

Hi Claes,

This is very good optimization. I have a few nit picks:

[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"));
     }


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


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


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

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

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

Thanks
- Ioi


On 4/13/20 12:22 PM, Claes Redestad wrote:
> Hi,
>
> during JDK 9 development we ended up converting package name Strings
> from external to internal form in the JNI layer rather than in Java - a
> reasonable optimization at the time[1].
>
> In light of recent startup optimization work, profiles now show clearly
> that we still have some overhead here. Partly due calling many times
> over into the VM from JNI. Moving the conversion completely into VM we
> reduce overhead of Module.defineModule0, with a measurable effect
> on bootstrap overhead[2].
>
> Patch: http://cr.openjdk.java.net/~redestad/8242452/open.00/
> Bug:   https://bugs.openjdk.java.net/browse/JDK-8242452
>
> Testing: tier1-6, complete headless run of JCK 15-b04 locally
>
> The patch includes a few enhancement to internal utilities, e.g., an
> as_utf8_string variant which allows (re-)using a provided char buffer if
> it's sufficiently sized, but which does not truncate the converted
> string if the buffer is too small. I'm open to splitting that out into a
> standalone RFE if preferable.
>
> Thanks!
>
> /Claes
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8171855
> [2]
> perf stat -r 250 java HelloWorld
>
> Before:
>        103,884,846 instructions # 0.81 insns per cycle ( +- 0.07% )
>         20,470,109 branches # 419.732 M/sec ( +- 0.07% )
>            740,708 branch-misses # 3.62% of all branches ( +- 0.14% )
>
>        0.033977482 seconds time elapsed ( +- 0.23% )
>
> After:
>        102,459,412 instructions # 0.80 insns per cycle ( +- 0.08% )
>         20,192,923 branches # 416.229 M/sec ( +- 0.08% )
>            733,137 branch-misses # 3.63% of all branches ( +- 0.13% )
>
>        0.033858386 seconds time elapsed ( +- 0.32% )