RFR: 8214799: Add package declaration to each JTREG test case in the gc folder

Fri Jan 11 16:54:48 UTC 2019

Hi again!

New webrev, no incremental this time either as I have changed all 
copyright years, all library annotations, and several new tests have 
been rebased.

Changes since last time:
1) The bad rebase artifact (bad rebase of moved file) 
CriticalNativeStress.java is now removed. I still left the small 
refactoring to a Java JNI "mirror" class as I think it is nicer than the 
old solution with several Java files in different packages mapping to 
the same JNI file.
2) All relative @library lines now uses the absolute "/" that will 
improve future refactoring. I still have them on separate lines if 
several paths are used -- my reason is that they diff better.
3) All new test cases in the new directory nvdimm have been fixed
4) A new test in directory epsilon has been fixed
5) All (Oracle) copyright years have been updated to 2019, I did not 
bother to add Oracle copyrights to the Epsilon/Shenandoah sources as my 
contribution is insignificant.
6) Shenandoah has landed, but I will not update all those test cases, I 
made this webrev too large already. I have tried to update all 
Shenandoah test cases that are located outside the shenendoah folder though.

(New) Webrev:
http://cr.openjdk.java.net/~lkorinth/8214799/02/

Testing:
open/test/hotspot/jtreg/:hotspot_gc

Thanks,
Leo

On 22/12/2018 12:10, Leo Korinth wrote:
> Hi!
> 
> On 21/12/2018 21:50, Leonid Mesnik wrote:
>> Hi
>>
>> Great change. The only a couple of comments
>>
>> 1) I see that you added a lot of relative links to include test-root 
>> as library like:
>>
>> 32  * @library ../../..
>>
>>
>> I think you might want to use
>>   @library  > or add to existing library test/lib like
>> @library /test/lib /
>>
>> instead. You might find a lot of examples in existing compiler and gc 
>> tests.
>>
>> It is the more common an unified approach  rather then using relative 
>> paths. Also it allows you to don't care when you move tests in 
>> different packages.
>>
> I will try that, it is certainly better. I think I will use a new 
> @library line though, it is easier to see the change.
>>
>> 2) New test in 
>> http://cr.openjdk.java.net/~lkorinth/8214799/01/test/hotspot/jtreg/gc/epsilon/CriticalNativeStress.new.java.html 
>>
>> looks confusing to me. It is copied from 
>> http://cr.openjdk.java.net/~lkorinth/8214799/01/test/hotspot/jtreg/gc/stress/CriticalNativeStress.new.java.html 
>>
>>
>> So now we have test CriticalNativeStress 
>> gc/epsilon/CriticalNativeStress.java and CriticalNativeStressEpsilon 
>> in gc/stress/CriticalNativeStress.java
>> The code in both files and jtreg tags looks pretty similar. Have you 
>> added it intentionally? Could you please explain a little bit purpose 
>> of this.
> 
> Many thanks for finding this fault of mine. The file was moved with the 
> Shenandoah push, and I must have rebased it badly although the move of 
> the corresponding c-file went through, strange...
> 
>> Other changes looks good.
>>
> 
> Thanks for taking a look at it!
> I will create a new webrev after the holiday.
> 
> /Leo
> 
>> Leonid
> 
> 
>>> On Dec 21, 2018, at 11:32 AM, Leo Korinth <leo.korinth at oracle.com
>>> <mailto:leo.korinth at oracle.com>> wrote:
>>>
>>> Hi again!
>>>
>>> I am adding Leonid and Erik to the discussion to see if my package 
>>> additions are in any way troublesome to the bigger picture. I believe 
>>> they are not. We are already using Java packages in some tests; this 
>>> change will just add them to more tests so that one can easily use an 
>>> IDE.
>>>
>>> Please ignore the old webrev as I had to rebase against the resent 
>>> Shenandoah push. It is easier to just read this version 
>>> (http://cr.openjdk.java.net/~lkorinth/8214799/01/). Unfortunately no 
>>> incremental version is given as this is a rebase.
>>>
>>> I have not had the time to update the test cases under the 
>>> gc/shenandoah folder (I am also afraid to make this change any 
>>> bigger), but I have tried to update the test cases that changed or 
>>> was added by the Shenandoah push (some Epsilon and Shenandoah tests 
>>> outside the gc/shenandoah folder).
>>>
>>> This rebase basically applies the same kind of changes to the added 
>>> test cases. One change that I would like you to look more carefully 
>>> at is the addition of a helper class CriticalNative.java that uses 
>>> JNI. Without this refactoring I could not have used the same c-file 
>>> as it was used in different packages (it is used from three different 
>>> test cases, and the package is part of the JNI export name). Please 
>>> take an extra look at that change.
>>>
>>> New webrev:
>>> http://cr.openjdk.java.net/~lkorinth/8214799/01/
>>>
>>> Testing:
>>> I have run jtreg tests under gc/epsilon and I have run :hotspot_gc 
>>> with Shenandoah in order not to break those test cases.
>>>
>>> Also started new run of tier1-3 and :hotspot_gc
>>>
>>> Thanks,
>>> Leo
>>>
>>> On 05/12/2018 14:44, Leo Korinth wrote:
>>>> Hi,
>>>> I have added package declarations to all jtreg tests in the gc 
>>>> folder (and sub folders). Previously most tests were not in a 
>>>> package (though some where), making it extremely hard to work with 
>>>> jtreg tests in an IDE.
>>>> The main change consists of adding a package declaration that 
>>>> corresponds to the directory the test is located in. This also makes 
>>>> it necessary to change @run annotations. Some test cases uses 
>>>> "common" code that is not located in the test lib. Those test cases 
>>>> now need to have a @library annotation that references the jtreg 
>>>> test root.
>>>> A few test cases had package visible classes with the same name in 
>>>> the same directory. That did work before, but does not work anymore, 
>>>> so I have prefixed those package visible classes with the public 
>>>> class name.
>>>> class TestVerifySilentlyRunSystemGC
>>>> class TestVerifySubSetRunSystemGC
>>>> class TestEagerReclaimHumongousRegionsReclaimRegionFast
>>>> class TestEagerReclaimHumongousRegionsClearMarkBitsReclaimRegionFast
>>>> class TestEagerReclaimHumongousRegionsWithRefsReclaimRegionFast
>>>> Where classes have been referenced using string literals, I have 
>>>> tried to use a .class.getName() instead (where possible).
>>>> I have tried to update copyrights to all files (also adding a 
>>>> missing comma after the second year in some files), and I did add 
>>>> missing copyright to TestFromCardCacheIndex.
>>>> known cons:
>>>> - needs a package line in each java file
>>>> - needs an extra library line in the test if the test is using other 
>>>> files.
>>>> - sometimes needs a run tag
>>>> improvements:
>>>> + can use eclipse or other IDEs without creating a workspace per 
>>>> test (I guess it will be a significant improvement for users of 
>>>> emacs and vi as well)
>>>> + can use test files located in other directories
>>>> After this change, I have several big cleanups that are easy to do 
>>>> with a working IDE.
>>>> Enhancement:
>>>> https://bugs.openjdk.java.net/browse/JDK-8214799
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~lkorinth/8214799/00/
>>>> Testing:
>>>> passed:
>>>> mach5 remote-build-and-test --build-profiles 
>>>> linux-x64,linux-x64-debug,macosx-x64,solaris-sparcv9,windows-x64 
>>>> --test open/test/hotspot/jtreg/:hotspot_gc
>>>> now running:
>>>> mach5 remote-test --build-id 2018-12-05-1132377.leo.korinth.hs 
>>>> --build-profiles linux-x64-debug -j tier1,tier2,tier3
>>>> Thanks,
>>>> Leo
>>