jmx-dev RFR 8020467: Inconsistency between usage.getUsed() and isUsageThresholdExceeded() with CMS Old Gen pool

[Jaroslav Bachorik]

Hi Bengt,

On 23.10.2013 14:40, Bengt Rutisson wrote:
>
> Hi Jaroslav,
>
> A couple of questions.
>
> I don't understand why this is a CMS only problem? Why don't the other
> collectors have the same issue? I guess it is less likely that the other
> collectors start (or complete) a GC without a lot of allocation going
> on. But at least G1 should have the same problem.

I don't really know. If there are other pools that can have the "used" 
value 0 before a GC happens then yes, they are susceptible to the same 
problem.

>
> Also, from the problem description in the CR I would have guessed that
> you want the GC to happen between these two statements:
>
> p.setUsageThreshold(1);
> MemoryUsage u = p.getUsage();

This is all but a heuristic here. The problem lies in the fact that it 
is not possible to retrieve the pool usage and the "threshold exceeded" 
flag consistently in one atomic operation. I might get usable data from 
the first call and then I don't need to force GC.

>
> Now you have added the GC just after these statements. I thought that
> was what caused the problem. That you read the usage data at one point,
> then a GC happens and you compare the cached usage
> data to the new data that p.isUsageThresholdExceeded() will fetch.
>
> Looking at the promoteToOldGen() method I assume that the intent is that
> the code should be using the return value. So my guess is that this code:
>
>    94         if (p.getName().equals("CMS Old Gen")) {
>    95             promoteToOldGen(p, u);
>    96         }
>
> Should be:
>
>    94         if (p.getName().equals("CMS Old Gen")) {
>    95             u = promoteToOldGen(p, u);
>    96         }

Indeed. It was meant to re-fetch the usage after GC.

>
> With that, I think it might work. But I still don't understand why this
> is only a CMS problem.
>
> One more question about the promoteToOldGen() and forceGC() methods. I
> don't really know much about how the different beans work, but are we
> sure that the MemoryPoolMXBeans and GarbageCollectorMXBeans use the same
> pool names? That is, are you sure that forceGC() actually will do anything?

They use the pool names as reported by the GC infrastracture so they 
should be the same.

>
> As for just doing a System.gc() to force a GC I think you can rely on
> that System.gc() does a full GC in Hotspot unless someone sets
> -XX:+DisableExplicitGC on the command line. Considering that you are
> relying on Hotspot specifc names for pools I don't think it is a
> limitation to the test to rely on the Hotspot implementatoin of
> System.gc().

Good to know. I guess I could simplify the change and just call 
System.gc(), after all.

Thanks,

-JB-

>
> Thanks,
> Bengt
>
>
>
>
> On 2013-10-23 10:18, Staffan Larsen wrote:
>> On 23 okt 2013, at 10:12, Jaroslav Bachorik
>> <jaroslav.bachorik at oracle.com> wrote:
>>
>>> On 23.10.2013 10:08, Staffan Larsen wrote:
>>>> I think you can simplify the logic for forcing a GC to just a simple
>>>> call to "System.gc();". AFAIK System.gc() will cause a full
>>>> collection to happen for all collectors.
>>> Hm, will it now? I had the impression that it was just hinting the GC
>>> system to perform GC but it might decide to ignore it. I need to be
>>> sure that the GC was performed before continuing - otherwise I might
>>> get inconsistent data again.
>> According to the spec it's just a hint, but I think the implementation
>> happens to be a force. But better safe than sorry. :)
>>
>> /Staffan
>>
>>> -JB-
>>>
>>>> /Staffan
>>>>
>>>> On 23 okt 2013, at 10:02, Jaroslav Bachorik
>>>> <jaroslav.bachorik at oracle.com> wrote:
>>>>
>>>>> On 22.10.2013 22:04, Mandy Chung wrote:
>>>>>> Hi Jaroslav,
>>>>>>
>>>>>> On 10/22/13 6:47 AM, Jaroslav Bachorik wrote:
>>>>>>> Please, review the following test fix:
>>>>>>>
>>>>>>> Issue:  https://bugs.openjdk.java.net/browse/JDK-8020467
>>>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8020467/webrev.01
>>>>>>>
>>>>>> Have you considered to force GC when getUsed() == 0 regardless of
>>>>>> which
>>>>>> memory pool it is?  This will avoid special casing for CMS old gen in
>>>>>> the test and will handle similar issue in the future for a different
>>>>>> collector implementation.  To make the test reliable, the test should
>>>>>> still pass if the memory pool has no object in it (G1 survivor space
>>>>>> case?).
>>>>> Hi Mandy,
>>>>>
>>>>> I don't know whether GC will help for other pools - but I can
>>>>> enable it for all pools - it should not hurt.
>>>>>
>>>>> The test should pass even with on object in the monitored pool
>>>>> since the pool should not report an exceeded threshold.
>>>>>
>>>>> -JB-
>>>>>
>>>>>> Mandy
>>>>>>
>>>>>>> The test tries to make sure that the "pool usage threshold" trigger
>>>>>>> and the reported pool memory usage are not contradicting each other.
>>>>>>> The problem is that it is not possible to get the "pool usage
>>>>>>> threshold exceeded" flag and the pool memory usage atomicly in
>>>>>>> regard
>>>>>>> to the GC. Specifically, when "CMS Old Gen" pool is examined and the
>>>>>>> usage is retrieved before a GC promotes some objects to the old gen
>>>>>>> but the usage threshold is checked after the GC has promoted some
>>>>>>> instance into the old gen the test will fail.
>>>>>>>
>>>>>>> The patch makes sure that there are some instances promoted in "CMS
>>>>>>> Old Gen" before checking the "pool usage threshold" to get
>>>>>>> semi-consistent view.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> -JB-
>