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

[serguei.spitsyn at oracle.com]

Hi Christoph,

The fix looks good to me.

Thanks,
Serguei


On 9/20/19 14:13, Langer, Christoph wrote:
> Thanks David!
>
> May I get another review?
>
> Best regards
> Christoph
>
>
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Donnerstag, 19. September 2019 13:56
>> To: Langer, Christoph <christoph.langer at sap.com>; Erik Joelsson
>> <erik.joelsson at oracle.com>; Magnus Ihse Bursie
>> <magnus.ihse.bursie at oracle.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 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
>>>>>