RFR: 8000975: (process) Merge UNIXProcess.java.bsd & UNIXProcess.java.linux (& .solaris & .aix)

Peter Levart peter.levart at gmail.com
Wed Apr 2 10:49:21 UTC 2014 

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.Bateman at 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.
>>>>>>>
>>>>>
>>>>
>>>
>>
>

More information about the core-libs-dev mailing list