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

[David Holmes]

Hi Erik,

Thanks for the additional details (I can't say I fully understand them :) ).

David

On 17/09/2019 11:39 pm, Erik Joelsson wrote:
> 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
>