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

Leo Korinth leo.korinth at oracle.com
Mon Jan 21 11:30:57 UTC 2019


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


More information about the hotspot-gc-dev mailing list