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