RFR: 8230857: Avoid reflection in sun.tools.common.ProcessHelper

[David Holmes]

Hi Magnus,

On 17/09/2019 9:26 pm, Magnus Ihse Bursie wrote:
> On 2019-09-17 01:01, David Holmes wrote:
>> Hi Christoph,
>>
>> Sorry for the delay getting back you.
>>
>> cc'd build-dev to get some clarification on the below ...
>>
>> On 12/09/2019 7:30 pm, Langer, Christoph wrote:
>>> Hi David,
>>>
>>>>> please review an enhancement which I've identified when working with
>>>>> Processhelper for JDK-8230850.
>>>>>
>>>>> I noticed that ProcessHelper is an interface in common code with a
>>>>> static method that would lookup the actual platform implementation via
>>>>> reflection. This seems a little cumbersome since we can have a common
>>>>> dummy for ProcessHelper and override it with the platform specific
>>>>> implementation, leveraging the build system.
>>>>
>>>> I don't see you leveraging the build system. You have two source files
>>>> that compile to the same destination class file. What is ensuring the
>>>> platform specific version is compiled after the generic one?
>>>>
>>>> Service-provider patterns use reflection to instantiate the service
>>>> implementation. I don't see any problem here that needs solving.
>>>
>>> TL;DR:
>>> There are two source files, one in share/classes and one in 
>>> linux/classes. The build system overrides the share/classes 
>>> implementation with the linux/classes implementation in the linux 
>>> build. This is not by coincidence and only one class is contained in 
>>> the generated jdk.jcmd module. Then there won't be a need for having 
>>> a service interface and a service implementation that is looked up 
>>> via reflection (which is not a bad pattern by itself). I agree that 
>>> it's not a big problem to be solved but still not "no problem".
>>> Here is some longer elaboration how the build system prefers specific 
>>> implementations of classes and filters generic duplicates:
>>> The SetupJavaCompilation function from JavaCompilation.gmk [0] is 
>>> used to compile the java sources for JDK modules. In its 
>>> documentation, for argument SRC [1], it claims: "one or more 
>>> directories to search for sources. The order of the source roots is 
>>> significant. The first found file of a certain name has priority". In 
>>> its implementation the found files are first ordered [3] and 
>>> duplicates filtered out [4].
>>> The potential source files are handed to SetupJavaCompilation in 
>>> CompileJavaModules.gmk [5] and were collected by a call to 
>>> FindModuleSrcDirs [6].  FindModuleSrcDirs iterates over all potential 
>>> source dirs for Java classes in the module [7]. The evaluated subdirs 
>>> are (in that order) $(OPENJDK_TARGET_OS)/classes, 
>>> $(OPENJDK_TARGET_OS_TYPE)/classes and share/classes, as per [8].
>>> Hope that explains what I'm trying to leverage here.
>>
>> I'm not 100% certain that what you describe actually ensures what you 
>> want it to ensure. I can't reconcile "the first found file ... has 
>> priority" with the fact found files are sorted and duplicates 
>> eliminated. It is the sorting that concerns me as it suggests 
>> linux/Foo.java might replace shared/Foo.java, but if we're on Windows 
>> then we have a problem! That said there is also this comment:
>>
>> # Order src files according to the order of the src dirs. Correct 
>> odering is
>> # needed for correct overriding between different source roots.
>>
>> I'd need the build team to clarify what "correct overriding" is 
>> actually defined as.
> David,
> 
> Christoph is correct. linux/Foo.java will override share/Foo.java. I 
> don't remember how the magic in JavaCompilation.gmk works anymore :-), 
> but we have relied on this behavior in other places for a long time, so 
> I'm pretty certain it is still working correctly. Presumably, the $(sort 
> ...) is there to remove (identical) duplicates, which is a side-effect 
> of sort.

Thanks for confirming. I'd still like to understand exactly what these 
overriding rules are though. It's not a mechanism I was aware of.

Thanks,
David

> /Magnus
> 
>>
>> Thanks,
>> David
>> -----
>>
>>> I've uploaded an updated webrev which contains some cleanup to the 
>>> Test changes: http://cr.openjdk.java.net/~clanger/webrevs/8230857.1/
>>>
>>> Thanks
>>> Christoph
>>>
>>> [0] 
>>> http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l185 
>>>
>>> [1] 
>>> http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l157 
>>>
>>> [3] 
>>> http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l225 
>>>
>>> [4] 
>>> http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l257 
>>>
>>> [5] 
>>> http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/CompileJavaModules.gmk#l603 
>>>
>>> [6] 
>>> http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/CompileJavaModules.gmk#l555 
>>>
>>> [7] 
>>> http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/Modules.gmk#l300 
>>>
>>> [8] 
>>> http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/Modules.gmk#l243 
>>>
>>>
>>>
>