RFR(XL): 8157957 - ClassNotFoundException: jdk.test.lib.JDKToolFinder

David Holmes david.holmes at oracle.com
Wed Aug 17 03:29:10 UTC 2016 

On 17/08/2016 9:58 AM, Igor Ignatyev wrote:
> Hi Christian,
>
> I’m sorry but I fail to see how absence of other users leads to ‘this class … shouldn't be in the shared code’. if we follow this logic, we will always have problems w/ having more than one user for a class and hence we won’t have any shared code. as I said dtrace testlibrary is a common testlibrary which can and should be used for other dtrace tests. so I don’t understand why you don’t want to place it in root/test.

I tend to agree. Utility code should be in the library to be used by 
future tests, with retrofitting applied to existing tests as resources 
permit. I see no point in actively removing anything from the shared 
library as part of this when the focus is on @build issues.

David
-----

> if the problem is in retesting, recreating/reuploading webrev, I’m fine w/ doing it by a separate changeset and I’d volunteer to do that.
>
> Thanks,
> — Igor
>> On Aug 16, 2016, at 11:52 PM, Christian Tornqvist <christian.tornqvist at oracle.com> wrote:
>>
>> Hi Igor,
>>
>>> dtrace testlibrary was created and put in common testlibrary directory
>> exactly in order to help porting our existing dtrace tests, having it
>> "hidden" in hotspot/test/compiler/codecache/dtrace can lead us to
>> reimplementing the same functionality. that is to say I think we should have
>> it in root/testlibrary.
>>
>> Since the runtime group is now responsible for dtrace, we'll record the
>> existence of this class for any future porting of the dtrace tests. Right
>> now, this class is not used by anyone except the codecache/dtrace test and
>> shouldn't be in the shared code.
>>
>> Thanks,
>> Christian
>>
>> -----Original Message-----
>> From: Igor Ignatyev [mailto:igor.ignatyev at oracle.com]
>> Sent: Tuesday, August 16, 2016 7:05 AM
>> To: David Holmes <david.holmes at oracle.com>; Christian Tornqvist
>> <christian.tornqvist at oracle.com>
>> Cc: hotspot-dev at openjdk.java.net Developers <hotspot-dev at openjdk.java.net>;
>> jdk9-dev <jdk9-dev at openjdk.java.net>
>> Subject: Re: RFR(XL): 8157957 - ClassNotFoundException:
>> jdk.test.lib.JDKToolFinder
>>
>> Christian/David,
>>
>>> Ah! I missed the significance of the renames ...
>>>
>>>> test/testlibrary/jdk/test/lib/dtrace/*
>>>
>>> Could be generally useful if all our dtrace tests were located
>>> together, but as they are not .
>>
>> dtrace testlibrary was created and put in common testlibrary directory
>> exactly in order to help porting our existing dtrace tests, having it
>> "hidden" in hotspot/test/compiler/codecache/dtrace can lead us to
>> reimplementing the same functionality. that is to say I think we should have
>> it in root/testlibrary.
>>
>> Thanks,
>> - Igor
>>
>>> On Aug 16, 2016, at 2:11 AM, David Holmes <david.holmes at oracle.com> wrote:
>>>
>>> Hi Christian,
>>>
>>> Thanks for bring a broader audience into this. For reference to others the
>> thread started with:
>>>
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-August/024198.
>>> html
>>>
>>> On 16/08/2016 1:42 AM, Christian Tornqvist wrote:
>>>> Hi David,
>>>>
>>>>> Do the jtreg authors agree with that rule? I'm happy to have such a
>>>>> simple
>>>> rule as long as it is agreed upon by all.
>>>> We've had some discussions about this, Jonathan says that explicit
>>>> @build is needed to support running with cached classes in jtwork and
>>>> modifying the classes in the @library path. This isn't something that
>>>> is a very common case in Hotspot and is avoidable by using a clean
>>>> jtwork dir when modifying these shared classes.
>>>>
>>>> Think of it this way, when you run javac on your program Main.java
>>>> that use the class A, you don't have to explicitly run javac on
>>>> A.java first, this is the way the javac works and there's nothing
>> different with jtreg here.
>>>
>>> It is a bit more complicated than that with libraries on different
>> sourcepaths, but I don't know the intricacies of javac's implicit
>> compilation checks. There are some obvious cases where reflection means
>> there is no compile-time dependency between classes, but I don't know if we
>> utilize anything of that nature in the test library (arguably we should in
>> places to ensure the library can function on compact profiles and with the
>> minimal VM - but that is a different problem).
>>>
>>> I remain very wary of this approach because I know we had to add @build to
>> fix some missing library class problems.
>>>
>>>>> Are these only: Platform.jdkLibPath(), Platform.sharedObjectName ?
>>>>> They seem general-purpose to me even if mosts tests don't need to care.
>>>>> So they seem suitable inclusions into the Platform class to me.
>>>>
>>>> No, I was mostly talking about:
>>>
>>> Ah! I missed the significance of the renames ...
>>>
>>>> test/testlibrary/jdk/test/lib/dtrace/*
>>>
>>> Could be generally useful if all our dtrace tests were located together,
>> but as they are not ...
>>>
>>>> test/testlibrary/jdk/test/lib/AllocationHelper.java
>>>
>>> Potentially useful. We have a lot of tests that try to consume/exhaust
>> memory, but whether they are amenable to conversion to use this utility is
>> another matter.
>>>
>>>> test/testlibrary/jdk/test/lib/HeapRegionUsageTool.java
>>>
>>> Potentially useful.
>>>
>>>> test/testlibrary/jdk/test/lib/InputArguments.java
>>>
>>> This seems particularly useful, but also seems to have some overlap with
>> existing argument processing code.
>>>
>>>> I can put back the jdkLibPath and sharedObjectName code if we believe
>>>> that this is something than more than one test will need. I think we
>>>> should be careful about putting unnecessary code in the shared classes.
>>>
>>> I don't have an issue with putting any kind of utility code (like the
>> above examples) into the shared test library if there is potential for
>> sharing. While retrofitting existing tests to use these things and remove
>> duplication would be a lot of effort, at least if they exist in the library
>> someone writing new tests and browsing the library won't be tempted to
>> re-invent the wheel.
>>>
>>> So I'd vote for not changing these.
>>>
>>>>> why is / listed as a library ??
>>>>
>>>> This is the compiler team's decision to use package name in their
>>>> tests and use / as the @library path, can't say I agree with it.
>>>
>>> Not sure I even understand what it means!
>>>
>>>>> Just to be crystal clear, did you also run all the @ignore'd tests?
>>>>
>>>> No, that was on my  list of things to do, which I forgot. Just ran it
>>>> and found issues with 2 of them that I corrected:
>>>>
>>>> * Missing imports of Platform and JDKToolFinder in
>>>> runtime/NMT/MallocStressTest.java
>>>> * Extra "}" in serviceability/dcmd/jvmti/LoadAgentDcmdTest.java
>>>
>>> Ok.
>>>
>>> You overlooked answering my query about the JDK test changes.
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Christian
>>>>
>>>> -----Original Message-----
>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>> Sent: Monday, August 15, 2016 12:58 AM
>>>> To: Christian Tornqvist <christian.tornqvist at oracle.com>;
>>>> hotspot-dev at openjdk.java.net
>>>> Subject: Re: RFR(XL): 8157957 - ClassNotFoundException:
>>>> jdk.test.lib.JDKToolFinder
>>>>
>>>> Hi Christian,
>>>>
>>>> First, thanks for attempting this, it is a huge effort!
>>>>
>>>> Second, as this is changing the <root>/test/lib files then it needs
>>>> to be reviewed by more than just the hotspot group.
>>>>
>>>> I won't claim to have examined every file in detail - there is a lot
>>>> that has to be taken on face-value here. I have skimmed the webrevs
>>>> and patch files.
>>>>
>>>> A few comments in-lined below ...
>>>>
>>>> On 13/08/2016 1:32 AM, Christian Tornqvist wrote:
>>>>> Hi everyone,
>>>>>
>>>>> Please review this fix for the CNFE issue we've had in the Hotspot
>>>>> jtreg tests. The two big culprits for the intermittent CNFE are:
>>>>> Duplicate classes on classpath with different content and @build
>>>>> tags causing class files to be written in bad places.
>>>>>
>>>>> There has been a lot of confusion around when there's a need to add
>>>>> @build to a test. In general it turns out it has been overused,
>>>>> which can lead to side effects. Here's an easy rule:
>>>>>
>>>>> If you run only that test in a clean jtwork folder and it passes,
>>>>> then there's no need for @build.
>>>>
>>>> Do the jtreg authors agree with that rule? I'm happy to have such a
>>>> simple rule as long as it is agreed upon by all.
>>>>
>>>>> If it doesn't pass, then it might need an explicit @build, here are
>>>>> some examples when it might be needed:
>>>>>
>>>>> * If the class is used by ClassFileInstaller and this is invoked by
>>>>> @run main ClassFileInstaller (sun.hotspot.Whitebox is an example of
>>>>> this)
>>>>>
>>>>> * If it's a class that that doesn't have reference from the Test
>>>>> itself (if "javac Test" wouldn't compile it, you might need to
>>>>> explicitly @build it)
>>>>>
>>>>> The change includes:
>>>>>
>>>>> * Removing all unnecessary @build tags, some of the CNFE was due to
>>>>> use of it to compile classes in /test/lib or /testlibrary when
>>>>> having different classpath (@library) set.
>>>>>
>>>>> * Changed @build <testname> to @build sun.hotspot.Whitebox
>>>>>
>>>>> * Moved /test/lib/share/classes/jdk/test/lib to
>>>>> /test/lib/jdk/test/lib
>>>>>
>>>>> * Some of the classes in /hotspot/test/testlibrary was only used by
>>>>> one or two tests, these classes shouldn't be part of the shared
>>>>> testlibrary code and they were moved to the location of the test
>>>>
>>>> Are these only: Platform.jdkLibPath(), Platform.sharedObjectName ?
>>>>
>>>> They seem general-purpose to me even if mosts tests don't need to care.
>>>> So they seem suitable inclusions into the Platform class to me.
>>>>
>>>>>
>>>>> * Merged/moved the classes in /hotspot/test/testlibrary to /test/lib
>>>>>
>>>>> * Moved some of the /test/lib classes into packages
>>>>>
>>>>> * Changed @library /testlibrary to @library /test/lib
>>>>
>>>> Related to that I see a number of entries like:
>>>>
>>>> - * @library /testlibrary /test/lib /
>>>> + * @library /test/lib /
>>>>
>>>> this is not part of your change by why is / listed as a library ??
>>>>
>>>>> * Changed imports from jdk.test.lib.* to explicit imports to help in
>>>>> future refactoring
>>>>>
>>>>> Almost all of the changes in here are in the jtreg @build and
>>>>> @library tags, very little code has been touched. Copyright headers
>>>>> will be updated before push.
>>>>>
>>>>> Testing done: Ran the entire hotspot/test/ folder multiple times
>>>>> locally and in RBT
>>>>
>>>> Just to be crystal clear, did you also run all the @ignore'd tests?
>>>>
>>>>> Webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~ctornqvi/webrev/8157957/webrev.00/
>>>>
>>>> The jdk/test/* changes only seem to be for the @library changes in
>>>> the path, I don't see any changes to @build tags. Is that because all
>>>> the @builds are necessary or are you simply not taking on the task of
>>>> also updating the jdk tests?
>>>>
>>>>> A recently added test required me to fix that as well, generating
>>>>> and uploading a new webrev of the Hotspot repo changes takes about
>>>>> 3h though, so here's the diff for that file:
>>>>>
>>>>> diff -r f37577c20a6b
>>>>> test/serviceability/sa/sadebugd/SADebugDTest.java
>>>>>
>>>>> --- a/test/serviceability/sa/sadebugd/SADebugDTest.java Wed Aug 10
>>>>> 21:02:14
>>>>> 2016 -0400
>>>>>
>>>>> +++ b/test/serviceability/sa/sadebugd/SADebugDTest.java Fri Aug 12
>>>>> +++ 11:25:54
>>>>> 2016 -0400
>>>>>
>>>>> @@ -26,7 +26,7 @@
>>>>>
>>>>> * @summary Checks that the jshdb debugd utility sucessfully starts
>>>>> *          and tries to attach to a running process
>>>>> * @modules java.base/jdk.internal.misc
>>>>>
>>>>> - * @library /test/lib/share/classes
>>>>> + * @library /test/lib
>>>>> *
>>>>> * @run main/othervm SADebugDTest
>>>>> */
>>>>
>>>> Looks fine.
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> Bug:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8157957
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Christian
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>
>>
>

More information about the jdk9-dev mailing list