Thank you, Serguei. I pushed it now: http://hg.openjdk.java.net/jdk/jdk/rev/577e17cab93f
> -----Original Message----- > From: serguei.spit...@oracle.com <serguei.spit...@oracle.com> > Sent: Samstag, 21. September 2019 09:13 > To: Langer, Christoph <christoph.lan...@sap.com>; David Holmes > <david.hol...@oracle.com>; OpenJDK Serviceability <serviceability- > d...@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.hol...@oracle.com> > >> Sent: Donnerstag, 19. September 2019 13:56 > >> To: Langer, Christoph <christoph.lan...@sap.com>; Erik Joelsson > >> <erik.joels...@oracle.com>; Magnus Ihse Bursie > >> <magnus.ihse.bur...@oracle.com>; OpenJDK Serviceability > <serviceability- > >> d...@openjdk.java.net>; build-dev <build-...@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.hol...@oracle.com> > >>>> Sent: Mittwoch, 18. September 2019 01:13 > >>>> To: Erik Joelsson <erik.joels...@oracle.com>; Magnus Ihse Bursie > >>>> <magnus.ihse.bur...@oracle.com>; Langer, Christoph > >>>> <christoph.lan...@sap.com>; OpenJDK Serviceability <serviceability- > >>>> d...@openjdk.java.net>; build-dev <build-...@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 > >>>>>
