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
