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

Leo Korinth leo.korinth at oracle.com
Thu Jan 17 15:58:34 UTC 2019


Hi,

I have added package name to newly added TestPeriodicLogMessages.java.

I would be happy for a review from a reviewer (for the whole change, not 
only this test).

The updated test case has been tested.

/Leo


diff --git a/test/hotspot/jtreg/gc/g1/TestPeriodicLogMessages.java 
b/test/hotspot/jtreg/gc/g1/TestPeriodicLogMessages.java
index 2cede583c45..bd02a6e1c0a 100644
--- a/test/hotspot/jtreg/gc/g1/TestPeriodicLogMessages.java
+++ b/test/hotspot/jtreg/gc/g1/TestPeriodicLogMessages.java
@@ -21,6 +21,8 @@
   * questions.
   */

+package gc.g1;
+
  /**
   * @test TestPeriodicLogMessages
   * @bug 8216490
@@ -29,6 +31,7 @@
   * @library /test/lib /
   * @modules java.base/jdk.internal.misc
   * @modules java.management/sun.management
+ * @run main gc.g1.TestPeriodicLogMessages
   */

  import jdk.test.lib.process.OutputAnalyzer;



On 14/01/2019 14:43, Leo Korinth wrote:
> On 11/01/2019 18:30, Leonid Mesnik wrote:
>> Hi
>>
>> The changes look good to me. Thanks for fixing all this.  Please note 
>> that I am not a 'R'eviewer.
>>
>> Leonid
> 
> Thank you Leonid for your (committer) review and suggestions for 
> improvement.
> 
> /Leo
> 
>>> On Jan 11, 2019, at 8:54 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
>>>
>>> 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
>>>>>
>>


More information about the hotspot-gc-dev mailing list