RFR: 8242452: During module definition, move conversion of packages from native to VM
Claes Redestad
claes.redestad at oracle.com
Thu Apr 16 19:56:03 UTC 2020
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
More information about the jigsaw-dev
mailing list