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