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
