On 15/12/2014 17:48, Daniel Fuchs wrote:
Hi guys,
Do I have approval to push the latest version of the test for JDK 9:
http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk9.02/
or are there still some objections?
I'm fine with the test Daniel (as before) - it's a long-life test that
can be tweaked and improved upon each iteration.
regards,
Sean.
best regards,
-- daniel
On 06/12/14 11:44, Daniel Fuchs wrote:
Hi Peter,
On 12/6/14 11:16 AM, Peter Levart wrote:
On 12/05/2014 09:06 PM, Daniel Fuchs wrote:
Hi David, all,
@David: You're right David.
The loader parameter is never used - I have removed it.
@all: I have received a comment from Alan that it would be better
to use
the new jrt:/ FileSystem directly, rather than using private
APIs.
One of the consequence is that the test now loads all the
classes in the runtime image (not just the ones in the BCL),
and therefore I have removed the toggle that allowed to test
the boot classes only.
http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk9.02/
best regards,
-- daniel
Hi Daniel,
Would it be possible to extract a minimal API for streaming over
classes and put that into the testlib so that only this part differs
between JDK8 and JDK9 and the guts of the test is identical for both
JDK8 and JDK9 ?
This would ease backport efforts when the test(s) are later "beefed
up" with other functionality. The API could later be extended with
other functionality, but just for illustration what might be needed,
here's what Martin Buchholz started as a test for finding possible
data races in reflection and I just re-packed using Streams. From
first glance it seems the tests need similar common functionality:
http://cr.openjdk.java.net/~plevart/misc/SunReflectDataRaces/
Regards, Peter
The test is already built like that:
The part that finds the class names is encapsulated in an object which
implements Iterable<String>.
http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk9.02/ ->
ClassNameJrtStreamBuilder
http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk8-00/ ->
ClassNameStreamBuilder
http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk7.00/ ->
ClassNameListBuilder
The bulk of the test should be identical (well, almost identical since I
only
updated 9 after the reviews, and since 9 is the only one that needs the
System ClassLoader)... I suppose we could extract these 3 classes and
define
a library class with something like:
public static class ClassNameFinder {
public static Iterable<String> findAllClassNames() {
jdk9 impl: return new ClassNameJrtStreamBuilder();
jdk8 impl: return new ClassNameStreamBuilder();
jdk7 impl: return new ClassNameListBuilder();
}
}
As it stands - this corresponds to the
static Iterable<String> listAllClassNames()
factory method in each version of the test.
best regards,
-- daniel
On 05/12/14 00:36, David Holmes wrote:
Hi Daniel,
I still find your use of the classloader very confusing. You talk
about
the defining loader but you never check the defining loader against
anything. In
146 static void checkFor(Class<?> c, ClassLoader loader) {
the loader variable is never used. And if loader is simply the
name of
the loader passed to forName, then it may not be the defining loader
anyway - so the whole use of this loader variable seems
superfluous at
best, and confusing/misleading at worst. And you can use the simple
forName(name) variant rather than passing a loader.
David
On 5/12/2014 3:13 AM, Daniel Fuchs wrote:
On 04/12/14 14:02, Seán Coffey wrote:
Thanks for driving efforts in this area Daniel. I think it's a very
useful test and is bound to help test code coverage. If it's
currently
passing on all JPRT platforms, it's a good measure.
It seems to :-)
Eventually I think we can bulk up the tests that can be run on the
Iterable returned from your class search.
At moment you just test Field.setAccessible.
Yes. If we change it later to test more, we will probably need to
change its name. I was already in lack of inspiration:
FieldSetAccessibleTest is not really a great name - but hopefully
it can do for now.
In the future, I don't see any harm in adding all simple Field
method
calls so that any corner cases in custom classes like the original
issue
are caught. e.g Field.getDeclaredAnnotations(),
Field.getModifiers(),
Field.isEnumConstant() etc., etc. Some methods won't be much value
add
but they're not a cost either.
Agreed.
Same argument for running through all Class methods, e.g
Class.getDeclaredClasses(), Class.getDeclaredMethods(). As a
result this
test might eventually become more general in test goal and might
live
better one level up in "test/java/lang/Class/" - it can be moved
when
the time comes.
Agreed as well :-)
Here is a new revision of the webrev for 9 in which I have
incorporated David's comment:
http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk9.01/
best regards,
-- daniel
regards,
Sean.
On 04/12/2014 12:25, Daniel Fuchs wrote:
On 04/12/14 13:06, David Holmes wrote:
Hi Daniel,
On 4/12/2014 9:38 PM, Daniel Fuchs wrote:
Hi David,
In fact I could use 'null' on JDK 9 as well.
My first version of the JDK 9 test was parsing over all the
.jimage
files under <jdk>/lib/modules - which explain why I needed to
use the System class loader.
Then I switched to only parsing the bootmodules.jimage - because
I noticed that the results where more coherent with what I had
observed on 8 & 7 - but I kept using the System class loader.
I am not sure whether we want the test for 9 should iterate over
the three .jimage - or continue to test only the boot .jimage.
I have updated the JDK 9 test (refreshed the webrev in place)
http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk9.00/
and added support for possibly running the test in the two modes
(I added a 'test.boot.only' system property, true by default)
as well as additional traces to print the loaded classes by
defining loader at the end (test.list.classes, false by
default).
A couple of initial comments:
104 static ClassLoader getClassLoaderFor(String
classFileName) {
105 if (restrictToBoot) return null; // only
bootmodules
106 return ClassLoaders.systemClassLoader;
107 }
I'm not clear the intent here. If it is to return a loader for
which
loadClass could be invoked then you can always just return the
system
loader - or just Class.forName. If it is meant to the return the
expected defining loader then it isn't doing that as the
extensions
loader is not allowed for.
The intent is to return the class loader that will be passed to
Class.forName( ). I've been fiddling & experimenting with this
test over 3 different platforms while trying to minimize the
differences - so that was my attempt at having a good place to
experiment with different strategy for loading classes.
Similarly for:
128 static ClassLoader getFor(String classFileName) {
129 return systemClassLoader;
130 }
Oh - that's my mistake. In fact ClassLoader getClassLoaderFor(..)
was supposed to simply return ClassLoaders.getFor(...);
and all the code should be in ClassLoaders.getFor - my bad.
Minor nit - In:
135 System.err.println("Unexpected loader for
"+c+":
"+c.getClassLoader());
c.getClassLoader() can be replaced by cl. Also put spaces around
the +
operator.
Good catch.
Thanks a lot David! Have a good night! (that's quite late - isn't
it?)
-- daniel
David
(signing off for the night)
Thanks for your question, it triggered me into looking deeper
into what was happening :-)
best regards,
-- daniel
On 04/12/14 10:05, Daniel Fuchs wrote:
The differences between 8 & 9 are limited to:
- ClassLoader:
- on 8 we use 'null' (BCL)
- on 9 we use the system class loader.
I haven't seen anything in JEP 220, regarding modules, that
indicates
that classes currently loaded by the boot-loader will now be
loaded
by the system classloader ???
In [1] towards the end:
[1] http://openjdk.java.net/jeps/220
"The defining class loader of the types in some existing
packages
will change. Existing code that makes assumptions about the
class
loaders of these types might not work correctly."
(then there is a list of specific changes).
This test looks up all class names in the image files and
attempt
to load the corresponding class. But as indicated in [1]
some of these classes are now in the Boot CL, some in the
Extension CL, and some in the Application CL.
So the test uses the System CL to load each class - which
ensures
that the loading will be delegated to the appropriate
ClassLoader.
best regards,
-- daniel
