[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: RE: RFR: 8230857: Avoid reflection in sun.tools.common.ProcessHelper
From: "Langer, Christoph" <christoph.langer () sap ! com>
Date: 2019-09-23 10:34:09
Message-ID: AM6PR02MB5192CBF7A0BDF0E2DA885E7B8A850 () AM6PR02MB5192 ! eurprd02 ! prod ! outlook ! com
[Download RAW message or body]
Thank you, Serguei.
I pushed it now: http://hg.openjdk.java.net/jdk/jdk/rev/577e17cab93f
> -----Original Message-----
> From: serguei.spitsyn@oracle.com <serguei.spitsyn@oracle.com>
> Sent: Samstag, 21. September 2019 09:13
> To: Langer, Christoph <christoph.langer@sap.com>; David Holmes
> <david.holmes@oracle.com>; OpenJDK Serviceability <serviceability-
> dev@openjdk.java.net>
> Subject: Re: RFR: 8230857: Avoid reflection in
> sun.tools.common.ProcessHelper
>
> 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@oracle.com>
> >> Sent: Donnerstag, 19. September 2019 13:56
> >> To: Langer, Christoph <christoph.langer@sap.com>; Erik Joelsson
> >> <erik.joelsson@oracle.com>; Magnus Ihse Bursie
> >> <magnus.ihse.bursie@oracle.com>; OpenJDK Serviceability
> <serviceability-
> >> dev@openjdk.java.net>; build-dev <build-dev@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@oracle.com>
> >>>> Sent: Mittwoch, 18. September 2019 01:13
> >>>> To: Erik Joelsson <erik.joelsson@oracle.com>; Magnus Ihse Bursie
> >>>> <magnus.ihse.bursie@oracle.com>; Langer, Christoph
> >>>> <christoph.langer@sap.com>; OpenJDK Serviceability <serviceability-
> >>>> dev@openjdk.java.net>; build-dev <build-dev@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
> >>>>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic