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

Thu Sep 19 09:47:16 UTC 2019

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/

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
> >