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

Tue Aug 16 11:04:32 UTC 2016

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
>>> 
>>> 
>>> 
>>> 
>>> 
>> 