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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Claes Redestad
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% )
Reply | Threaded
Open this post in threaded view
|

Re: 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% )

Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman


On 16/04/2020 00:04, Ioi Lam wrote:
> 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.
It would be a bug if these JVM_XXX functions were called with null or
otherwise invalid values. So additional checks shouldn't be needed, they
would be just a safety net. That said, I thought the white box tests
covered these cases so that the JVM functions could be tested with junk
input to make sure that they were robust.

-Alan
Reply | Threaded
Open this post in threaded view
|

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

Claes Redestad
Hi,

my intent has been to not remove any checks in the VM endpoints that
were there previously. Some of the package == NULL checks might not be
as immediately apparent, but they should all be there (and checked for
by pre-existing tests such as runtime/modules/JVMAddModuleExports.java)

/Claes

On 2020-04-16 13:17, Alan Bateman wrote:

>
>
> On 16/04/2020 00:04, Ioi Lam wrote:
>> 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.
> It would be a bug if these JVM_XXX functions were called with null or
> otherwise invalid values. So additional checks shouldn't be needed, they
> would be just a safety net. That said, I thought the white box tests
> covered these cases so that the JVM functions could be tested with junk
> input to make sure that they were robust.
>
> -Alan
Reply | Threaded
Open this post in threaded view
|

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

Claes Redestad
In reply to this post by Ioi Lam
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
Reply | Threaded
Open this post in threaded view
|

Re: 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

Reply | Threaded
Open this post in threaded view
|

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

Claes Redestad


On 2020-04-16 23:05, Ioi Lam wrote:
>
> 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.
>

Fixed - a minor mishap.

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

Updated in-place with the jvmti changes. Re-ran relevant tests locally,
re-running tier1+2.

/Claes
Reply | Threaded
Open this post in threaded view
|

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

Claes Redestad
In reply to this post by Claes Redestad
Hi Harold,

On 2020-04-17 15:32, Harold Seigel wrote:
> Hi Claes,
>
> The change looks good.  Just a couple of minor things.

thank you for reviewing!

>
> In JavaClasses.cpp, line 652, does length need to be initialized to zero?

Good point, most of the length variables passed by reference to retrieve
the length can be left uninitialized.

>
> The indentation looks wrong at line 658.

Hmm, yeah. Not sure how to make it look right, though.

>
> The changes in modules.cpp look good!  Maybe as_symbol() could
> eventually be moved to symbolTable.hpp.

Seems reasonable, but as I want to avoid Handle'ing the oop I felt more
comfortable with a local utility method for now.

>
> Very minor typos in modules.hpp ("to to").

Fixed both places.

http://cr.openjdk.java.net/~redestad/8242452/open.02/

Have verified VM module tests locally and re-running a sanity tier1
before push.

>
> I don't need to see another webrev.
>
>
> One change that we looked at but did not do is to have one call from the
> JDK to the JVM to, for example, add all of a module's qualified
> exports.  Currently, each export is a separate call from the JDK to the
> VM.  I'm not sure if this is worth doing.

Could be, but I think we'd have to refactor the interaction more to be
able to turn it into a real gain.

A less granular API might be better suited for archiving optimizations,
which I know Ioi has been giving some thought to. That is: we're able to
archive the java object model for module descriptors and the default
model graph - why not also the VM model? Not today, but let's keep the
door open. :-)

/Claes