Hi David,
Thanks for the review please see embedded comments:
Hi Kumar,
On 8/11/2016 4:47 AM, Kumar Srinivasan wrote:
Hello,
Please review the fix for:
https://bugs.openjdk.java.net/browse/JDK-8169001
Webrev at:
http://cr.openjdk.java.net/~ksrini/8169001/webrev.00/
Overall this looks like a complete eradication of the launcher
ergonomics. A few specific comments:
src/java.base/share/native/launcher/main.c
! const_cpwildcard, const_javaw, 0);
Can you clarify this change with
! const_cpwildcard, const_javaw, 0 /* unused */);
To clarify:
-159 const_cpwildcard, const_javaw, const_ergo_class);
+159 const_cpwildcard, const_javaw, 0);
JLI_Launch is an *internal* entry point, this is primarily used in main.c,
and this is called by JDK's tool launchers which sets "NEVER_ACT_AS_SERVER",
also this entry point is used by the java packager and deployment, I
have cc'ed
Chris Bensen and David DeHaven, my take is not to change the signature now.
Chris, David ?
Thanks.
---
src/java.base/share/native/libjli/java.c
220 jint ergo /* unused */
I assume JLI_Launch is an exported API otherwise I'd expect this to be
deleted. The fact it is unused should also be documented in
src/java.base/share/native/libjli/java.h. But I'm also wondering where
this API change is being documented for external users?
This entry point is for internal users only.
- FreeKnownVMs(); /* after last possible PrintUsage() */
Not clear why you no longer need to free here. But if you don't then
FreeKnownVMs() seems to be dead code now.
Ugh (and thanks!), I was too aggressive, this should remain, I will put
it back in the next revision.
---
src/java.base/unix/conf/i586/jvm.cfg
I think you still need an entry for "-client KNOWN" as anyone can
build the client VM if they choose. Ditto for
src/java.base/unix/conf/sparc/jvm.cfg (though 32-bit Solaris builds
are obsolete now).
I had to mull on this, one hand jvm.cfg should reflect the VMs we support,
but we could have someone build client vm. I will go with making the change
you suggested.
---
You have updated a number of copyright notices but not all of them.
Yes, I vaguely recall, that the CR should be updated if there is any content
that ought to be copyrighted, in this case several changes are typo
fixups in the
comments. So I don't think it is warranted.
---
I see no changes to tests, but I'm pretty sure we have a test for
server-class-machine somewhere (or we did - I recall it breaking at
some point).
I ran -testset pit, saw no failures, I will perform a -testset hotspot.
Note though, several test changes had to be made, when Solaris 32-bit
was yanked out.
Thanks
Kumar
Thanks,
David
Background:
Launcher ergonomics was introduced last decade to help determine
if the execution system is "Server Class", this was necessary to
choose server VM on platforms that supported both client and server
VMs (primarily for Solaris and Linux 32-bit).
The algorithm involves computing and detecting the number of CPUs
and the amount of memory on the target system. All modern computers
systems with hyper-threading cause the ergonomics to choose server.
JDK9 Platforms that have only server vm.
./linux-x64/lib/amd64/server/libjvm.so
./linux-arm64-vfp-hflt/lib/aarch64/server/libjvm.so
./solaris-sparcv9/lib/sparcv9/server/libjvm.so
./solaris-x64/lib/amd64/server/libjvm.so
./windows-x86/bin/server/jvm.dll
./windows-x64/bin/server/jvm.dll
JDK9 Platforms that have more than one vm variant:
./linux-arm32-vfp-hflt/lib/arm/client/libjvm.so (default)
./linux-arm32-vfp-hflt/lib/arm/minimal/libjvm.so
./linux-x86/lib/i386/server/libjvm.so (default)
./linux-x86/lib/i386/minimal/libjvm.so
In the cases where multiple VMs are supported the ergnomics
has no effect, and the default platforms are chosen by the
jvm.cfg. Thus the launcher ergonomics is obsolete and redundant.
Thanks
Kumar
