[9 + 8u] RFR - 8066612: Add a test that will call getDeclaredFields() on all classes and try to set them accessible.
Daniel Fuchs
daniel.fuchs at oracle.com
Tue Dec 16 10:34:43 UTC 2014
Hi David,
On 12/16/14 6:20 AM, David Holmes wrote:
> On 16/12/2014 3:48 AM, 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?
>
> My objections stand - the classloader aspect of the test is overly
> complicated and not achieving anything.
I see. Sorry for misunderstanding. See below:
On 12/5/14 12:36 AM, David Holmes wrote:
> 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.
- I thought your objection was only with the loader argument in
checkClassLoaderFor,
which I removed in webrev-jdk9.02. I have prepared a new webrev.
- Using the simple Class.forName(name) is not an option because it
forces the
static initializers to run at class loading. I don't think we want
that in this test.
Here is a new version where I have removed all the 'ClassLoaders' stuff.
After all
it was not instrumental to the test. Is that good to go?
best regards,
-- daniel
>
> David
>
>> 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
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
More information about the jdk8u-dev
mailing list