[9 + 8u] RFR - 8066612: Add a test that will call getDeclaredFields() on all classes and try to set them accessible.
David Holmes
david.holmes at oracle.com
Tue Dec 16 05:20:55 UTC 2014
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.
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 core-libs-dev
mailing list