On 04/01/2014 09:47 PM, roger riggs wrote:
Hi,
A minor point, but the Enum for LaunchMechanism can be simpler; the
defined enum values (1,2,3)
are never used and can be removed along with the extra constructor.
They are used for the "mode" parameter of forkAndExec() native method.
With the refactoring so f0ar, this seems more complex and harder to
understand.
Are you comparing webrev.03 with webrev.04 or original code with
webrev.0[34] ?
I agree that one individual original source is simpler than merged
single source, but there are 4 of them to be kept in-sync and still be
different in places. I find it more convenient to see the differences in
one place without using a diff tool pair-wisely. It's also friendlier to
IDEs if they "understand" the code instead of just treating those 4
files as texts. Some of the IDEs can be teached to understand the
various (.java.bsd, .java.linux, .java.solaris, .java.aix) extensions as
Java sources, but then they get confused because they see 4 sources for
the same class...
At least in the non-merged version all (and only) the code for a
platform was in a single class.
The static UNIXProcess subclasses for the various platforms are always
kept around.
We could bring them to the upper level as package-private subclasses and
arrange in makefile to just include the ones that are needed. But then
this knowledge of mapping is in two places: the makefiles and the code.
Other alternatives would have been to factor the common code (Streams
handling)
into a utilities class or ProcessImpl and retain the 1st class
subclasses (with different names)
for each platform or merge more up into ProcessImpl.
Maybe it will be clearer with additional refactoring.
As I said, I believe the consolidation of various Input/OutputStream
wrappers could bring the class files number and size further down.
$.02, Roger
If you're concerned about class files included in the distributable, but
not used, we can compensate this a bit by reducing the number of
anonymous inner classes generated by javac just by replacing them with
lambdas. Here's new webrev that does that too:
http://cr.openjdk.java.net/~plevart/jdk9-dev/UNIXProcess/webrev.05/
If UNIXProcess.java in above webrev is compiled, the following class
files are produced:
-rw-rw-r--. 1 peter peter 772 Apr 2 12:33 UNIXProcess$1.class//
SwitchMap for Platform enum
-rw-rw-r--. 1 peter peter 2706 Apr 2 12:33 UNIXProcess$AixProcess.class
-rw-rw-r--. 1 peter peter 2155 Apr 2 12:33
UNIXProcess$DeferredCloseInputStream.class
-rw-rw-r--. 1 peter peter 2930 Apr 2 12:33
UNIXProcess$DeferredCloseProcessPipeInputStream.class
-rw-rw-r--. 1 peter peter 1166 Apr 2 12:33
UNIXProcess$LaunchMechanism.class
-rw-rw-r--. 1 peter peter 2701 Apr 2 12:33
UNIXProcess$LinuxOrBsdProcess.class
-rw-rw-r--. 1 peter peter 4762 Apr 2 12:33 UNIXProcess$Platform.class
-rw-rw-r--. 1 peter peter 1711 Apr 2 12:33
UNIXProcess$ProcessPipeInputStream.class
-rw-rw-r--. 1 peter peter 949 Apr 2 12:33
UNIXProcess$ProcessPipeOutputStream.class
-rw-rw-r--. 1 peter peter 1902 Apr 2 12:33
UNIXProcess$ProcessReaperThreadFactory.class
-rw-rw-r--. 1 peter peter 2935 Apr 2 12:33 UNIXProcess$SolarisProcess.class
-rw-rw-r--. 1 peter peter 6260 Apr 2 12:33 UNIXProcess.class
...12 class files totaling 30.2 KiB.
If original UNIXProcess.java.linux is compiled, for example, the
following files are produced:
-rw-rw-r--. 1 peter peter 1648 Apr 2 12:25 UNIXProcess$1.class
-rw-rw-r--. 1 peter peter 926 Apr 2 12:25 UNIXProcess$2.class
-rw-rw-r--. 1 peter peter 865 Apr 2 12:25 UNIXProcess$3.class
-rw-rw-r--. 1 peter peter 648 Apr 2 12:25 UNIXProcess$4.class
-rw-rw-r--. 1 peter peter 1200 Apr 2 12:25
UNIXProcess$LaunchMechanism.class
-rw-rw-r--. 1 peter peter 1711 Apr 2 12:25
UNIXProcess$ProcessPipeInputStream.class
-rw-rw-r--. 1 peter peter 949 Apr 2 12:25
UNIXProcess$ProcessPipeOutputStream.class
-rw-rw-r--. 1 peter peter 939 Apr 2 12:25
UNIXProcess$ProcessReaperThreadFactory$1.class
-rw-rw-r--. 1 peter peter 1233 Apr 2 12:25
UNIXProcess$ProcessReaperThreadFactory.class
-rw-rw-r--. 1 peter peter 5626 Apr 2 12:25 UNIXProcess.class
...10 class files totaling 15,4 KiB
So it's ~15 KiB that we are talking about at this moment.
Regards, Peter
On 4/1/2014 1:04 PM, Peter Levart wrote:
On 04/01/2014 05:43 PM, Peter Levart wrote:
On 04/01/2014 03:49 PM, roger riggs wrote:
Hi Peter,
The design using enum for the os dependencies does not make it
possible
to include only the support needed for a particular platform at
build time.
Every implementation will be carrying around the support for all
the other platforms.
A build time binding would be more efficient.
Roger
That's true. A trade-off between maintainability and efficiency. The
efficiency has two categories here. One is the size of the
distributable and the other is run-time efficiency. I've been
thinking to improve both efficiencies (the run-time in particular)
with a little re-design. Since nearly each OS platform requires a
sub-class of UNIXProcess to implement the differences, I can move
the implementations of various methods now in Os enum to the
UNIXProcess subclasses and get rid of Os enum per-instance subclasses.
Let me try this and see what comes out.
Hi Roger,
Well, it turns out the methods would like to stay in Os (renamed to
Platform), but there is no need for per-enum-instance subclasses.
Using enum constructor parameters and switch statements makes code
even more compact and easy to follow...
http://cr.openjdk.java.net/~plevart/jdk9-dev/UNIXProcess/webrev.04/
I belive there is still room for consolidating logic in various
Input/OutputStream wrappers used in UNIXProcess variants. But in the
first round I tried to preserve the exact behaviour. If the wrapping
of streams could be made more-or-less equal in all UNIX platforms,
then the need for UNIXProcess subclasses and/or overhead of support
classes included but not used goes away...
Regards, Peter
On 4/1/2014 9:16 AM, Peter Levart wrote:
Hi Alan, Volker,
Thanks for sharing the info and for testing on AIX. Here's the
updated webrev that hopefully includes the correct "dispatch on
os.name" logic. I included "Solaris" as an alternative to "SunOS"
since I saw this in some documents on Internet. If this is
superfluous, I can remove it:
http://cr.openjdk.java.net/~plevart/jdk9-dev/UNIXProcess/webrev.03/
I tested this on Linux and Solaris and the
java/lang/ProcessBuilder jtreg tests pass. So with Volker's test
on AIX, the only OS platform left for testing is Mac OS X. Would
someone volunteer?
Regards, Peter
On 03/27/2014 11:18 AM, Volker Simonis wrote:
Hi Peter,
thanks for applying these changes to the AIX files as well.
With the additional line:
if (osName.equals("AIX")) { return AIX; }
in Os.get() your change compiles cleanly on AIX and runs the
java/lang/ProcessBuilder tests without any errors.
So from an AIX perspective, thumbs up.
Regards,
Volker
On Wed, Mar 26, 2014 at 5:18 PM, Alan Bateman
<alan.bate...@oracle.com> wrote:
On 26/03/2014 15:19, Peter Levart wrote:
I couldn't find any official document about possible os.name
values for
different supported OSes. Does anyone have a pointer?
I don't know if there is a definite list but I assume we don't
need to be
concerned with anything beyond the 4 that we have in OpenJDK,
which is
"Linux", "SunOS", "AIX" and contains("OS X").
If we get to the point in JDK 9 where src/solaris is renamed to
src/unix (or
something equivalent) then it could mean that the Os enum can be
replaced
with an OS specific class in src/linux, src/solaris, ... and
this would
avoid the need for an os.name check at runtime.
-Alan.
