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

Thu Sep 19 11:56:13 UTC 2019

Hi Christoph,

On 19/09/2019 7:47 pm, Langer, Christoph wrote:
> Hi,
> 
> @Erik, Magnus: Thanks for stepping in to explain things.
> 
> Now back to the actual change: Is this ok then (@David)? Any other reviews from somebody else?
> 
> http://cr.openjdk.java.net/~clanger/webrevs/8230857.1/

It seems okay.

For the test I'm unclear on exactly how to ensure things are accessible, 
but presumably the +open is sufficient and works under all circumstances.

Thanks,
David

> Thank you!
> 
> Best regards
> Christoph
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Mittwoch, 18. September 2019 01:13
>> To: Erik Joelsson <erik.joelsson at oracle.com>; Magnus Ihse Bursie
>> <magnus.ihse.bursie at oracle.com>; Langer, Christoph
>> <christoph.langer at sap.com>; OpenJDK Serviceability <serviceability-
>> dev at openjdk.java.net>; build-dev <build-dev at openjdk.java.net>
>> Subject: Re: RFR: 8230857: Avoid reflection in
>> sun.tools.common.ProcessHelper
>>
>> 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
>>>