[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
Mon Dec 15 17:48:36 UTC 2014
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?
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