RFR: 8217389: JTREG: Clean up, remove unused variable warnings

[Leonid Mesnik]

Hi

Looks good, thanks for very nice clean up. Indeed, the  Reference.reachabilityFence() should be the best way to ensure that object is reachable. 

Leonid

> On Jan 21, 2019, at 3:30 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
> 
> Hi,
> 
> I have a new update that uses Reference.reachabilityFence to make sure that allocations are not optimized away. After a bit of thinking and asking around, I believe reachabilityFence to be a better solution than a public static variable in these cases where we want to make sure allocation is not optimized away but do not want the object to be reachable. It also makes it a bit more obvious that the code is used to allocate garbage and not to be optimized away rather than being a programming mistake.
> 
> I also added spaces to the "XXX*M" pattern.
> 
> Webrev (Full): http://cr.openjdk.java.net/~lkorinth/8217389/01
> Webrev (Incremental): http://cr.openjdk.java.net/~lkorinth/8217389/00_01
> 
> Testing: gc test suite still passes.
> 
> Thanks,
> Leo
> 
> On 21/01/2019 10:34, Leo Korinth wrote:
>> Hi!
>> On 18/01/2019 22:24, Leonid Mesnik wrote:
>>> Leo
>>> 
>>> 1. http://cr.openjdk.java.net/~lkorinth/8217389/00/test/hotspot/jtreg/gc/arguments/TestMaxNewSize.java.udiff.html 
>>> 
>>> I am not sure how your changes make code clear. Also '128*M' should have spaces.
>> The main purpose is to remove a compiler warning for unused variable (M64). I also do believe 128*M is more obvious at inspection than M128 (and the construct is used in many places in the code). It also saves two lines of code. I agree that 128*M should have spaces, I have seen it without spaces with the same type of construct, but now I see it with spaces as well. I will add the spaces.
>>> 
>>>> On Jan 18, 2019, at 9:23 AM, Leo Korinth <leo.korinth at oracle.com
>>>> <mailto:leo.korinth at oracle.com>> wrote:
>>>> 
>>>> Hi,
>>>> 
>>>> Here I am removing warnings for unused variables.
>>>> 
>>>> Some variables can not be removed as in the case of creating a new primitive array without assigning it to a variable. It is not allowed, in those cases I use @SuppressWarnings("unused").
>>>> 
>>> 
>>> I think that it would be better to really use these variables somehow.
>> Yes, but I am only removing warnings or bugs (or I will never finish this).
>>> The hotspot compilers might also optimize such arrays initialization since results are not used.  The tests like this
>>> http://cr.openjdk.java.net/~lkorinth/8217389/00/test/hotspot/jtreg/gc/cslocker/TestCSLocker.java 
>>> just silently pass without any memory allocation and actual testing.
>>> 
>>> I think it would be better to change this variables to be public static and/or be used by some external stuff (print zero/last element as example). It guarantee that compiler can't remove this allocation as a dead code.
>> Yes, I will make them public static variables. I understand that is enough to guarantee allocation. I will try to update these test cases accordingly. We will then get some actual improvement in tests and not only clear warnings :-)
>> I will prepare a new webrev.
>> Thanks, Leo
>>> 
>>> See following test as example:
>>> http://hg.openjdk.java.net/jdk/jdk/file/cb7347310fa1/test/hotspot/jtreg/gc/TestMemoryInitialization.java 
>>> 
>>> Might be some library helper which produce some amount off garbage is useful.
>>> There are already a lot of methods to produce garbage here but they seems to be overcomplicated. Not sure it make sense to reuse them.
>>> http://hg.openjdk.java.net/jdk/jdk/file/cb7347310fa1/test/hotspot/jtreg/vmTestbase/nsk/share/gc/gp 
>>> 
>>> 
>>> Leonid
>>> 
>>>> In some cases the variable is not used at all, and I remove it, but still add the variable name as a comment after the expression, as the variable name might have helped as documentation. I also just commented out "final int CONSTANT_Unicode = 2;" if that information might be useful in the future, it is a constant that we do not expect in the test case.
>>>> 
>>>> Otherwise I just try to remove the code.
>>>> 
>>>> This fix is somewhat based/dependent on the rest of my "JTREG:" webrevs.
>>>> 
>>>> Enhancement:
>>>> https://bugs.openjdk.java.net/browse/JDK-8217389
>>>> 
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~lkorinth/8217389/00
>>>> 
>>>> Testing:
>>>> passed locally: open/test/hotspot/jtreg/:hotspot_gc
>>>> 
>>>> Thanks,
>>>> Leo
>>>