RFR(S) 8200746: Remove FlagGuard class

[Gerard Ziemski]

Thank you Erik, David for the review.

My motivation for removing FlagGuard is that it’s not used anywhere in the hotspot itself.

It is used in test_globals.cpp, but that tests the functionality itself, so as Erik points out without the feature the test is no needed.

The test_collectorPolicy.cpp is the real issue here, which I missed, as I assumed the tests would be executed in fresh VM instances, but now I see that we should either:

1. Use “TEST_OTHER_VM” for the tests that modify the global flags
2. Implement FlagGuard in the test itself.

My main issue here is that we have code in hotspot, which is not used by hotspot (the product) itself, but only by one of its test. Furthermore, the code seems simple enough that it shouldn’t be too much trouble to provide an implementation of it in any test that needs it.

Erik, as the original author, do you think #1 will work for test_collectorPolicy.cpp, or do we need #2?


cheers


> On May 17, 2018, at 2:04 AM, Erik Helin <erik.helin at oracle.com> wrote:
> 
> On 05/17/2018 12:06 AM, David Holmes wrote:
>> Hi Gerard,
>> On 17/05/2018 4:06 AM, Gerard Ziemski wrote:
>>> Hi all,
>>> 
>>> Please review this small fix where we remove unused FlagGuard class and FLAG_GUARD macro from hotspot and gtests.
>> Well it seems only unused after you have removed its uses! Why does it turn out not to be needed in test_CollectorPolicy.cpp?
> 
> I think it is still needed, I don't think this patch is correct.
> 
> When we execute the gtests we execute the TEST_VM tests one-by-one in serial order in the same process. This means that if one tests changes a global flag, e.g. InitialHeapSize, the next test will see a different value for InitialHeapSize. In order to guard the flags, I introduced the class FlagGuard (and corresponding helper macro FLAG_GUARD). This means that a test, such as test_collectorPolicy.cpp, can easily guard any global flags the test intends to mutate, and then the flags will be restored to their original values when test has finished.
> 
>> Why is the whole test_globals.cpp no longer relevant?
> 
> test_globals.cpp currently only tests the FlagGuard class (and FLAG_GUARD) macro in globals.hpp, so since Gerard removed the FlagGuard class, the tests are no longer relevant. However, see my comment above, we can't just remove the FlagGuard class, it is needed!
> 
>> The bug report should explain why we don't need these.
> 
> Yes, I would also like to see an explanation why this code can be removed? To me it seems like it is needed in test_collectorPolicy.cpp, but I could be wrong.
> 
> Thanks,
> Erik
> 
>> Thanks,
>> David
>>> https://bugs.openjdk.java.net/browse/JDK-8200746
>>> http://cr.openjdk.java.net/~gziemski/8200746_rev1/
>>> 
>>> Testing: mach5 hs-tier1,2
>>> 
>>> 
>>> cheers
>>>