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