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
