RFR(XL): 8157957 - ClassNotFoundException: jdk.test.lib.JDKToolFinder
Igor Ignatyev
igor.ignatyev at oracle.com
Tue Aug 16 23:58:21 UTC 2016
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.
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