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

[Erik Joelsson]

Hello,

On 2019-09-17 05:59, David Holmes wrote:
> 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.
>
SetupJavaCompilation is indeed behaving as Christoph describes and it is 
by design. I implemented support for this behavior in:

https://bugs.openjdk.java.net/browse/JDK-8079344

The relevant parts of SetupJavaCompilation look like this:

   # Order src files according to the order of the src dirs. Correct 
odering is
   # needed for correct overriding between different source roots.
   $1_ALL_SRC_RAW := $$(call FindFiles, $$($1_SRC))
   $1_ALL_SRCS := $$($1_EXTRA_FILES) \
       $$(foreach d, $$($1_SRC), $$(filter $$d%, $$($1_ALL_SRC_RAW)))

The second line orders the src files by the src roots. (We used to just 
call find for one src root at a time, but the above actually performs 
better due only running 1 external process)

Further down we have this:

   ifneq ($$($1_KEEP_DUPS), true)
     # Remove duplicate source files by keeping the first found of each 
duplicate.
     # This allows for automatic overrides with custom or platform 
specific versions
     # source files.
     #
     # For the smart javac wrapper case, add each removed file to an 
extra exclude
     # file list to prevent sjavac from finding duplicate sources.
     $1_SRCS := $$(strip $$(foreach s, $$($1_SRCS), \
         $$(eval relative_src := $$(call remove-prefixes, $$($1_SRC), 
$$(s))) \
         $$(if $$($1_$$(relative_src)), \
           $$(eval $1_SJAVAC_EXCLUDE_FILES += $$(s)), \
           $$(eval $1_$$(relative_src) := 1) $$(s))))
   endif

This loop is a bit hairy to wrap your head around. It's iterating over 
all the src files, in the order of importance. The variable relative_src 
is the path from the src root, the part that is common to all duplicate 
src files. The variables on the form $1_$$(relative_src) basically act 
as a hash map (string->boolean). So for each src file, if the relative 
path for it has already been seen, add it to an exclude list, else mark 
it as seen and add it to the return list.

/Erik