metaspace.cpp: why are counters in ChunkManager updated atomically?

[Thomas Stüfe]

On Thu, Oct 27, 2016 at 3:26 PM, Erik Helin <erik.helin at oracle.com> wrote:

> On 10/27/2016 02:56 PM, Thomas Stüfe wrote:
>
>> Hi Mikael,
>>
>> On Thu, Oct 27, 2016 at 2:41 PM, Mikael Gerdin <mikael.gerdin at oracle.com>
>> wrote:
>>
>> Hi Thomas,
>>>
>>> On 2016-10-27 14:08, Thomas Stüfe wrote:
>>>
>>> Hi all,
>>>>
>>>> I am currently working on a prototype for
>>>> https://bugs.openjdk.java.net/browse/JDK-8166690 and have a question
>>>> about
>>>> the ChunkManager class in metaspace.cpp.
>>>>
>>>> ChunkManager has the _free_chunks_total, _free_chunks_count counters. It
>>>> seems the coding goes some lengths to avoid updating them often, so
>>>> instead
>>>> of updating them when a chunk is freed it attemps to delay and
>>>> accumulate
>>>> updates. This makes changing the MetaChunk allocation quite complicated,
>>>> because there are large windows during which the counters are invalid
>>>> and
>>>> do not reflect reality.
>>>>
>>>> I see that the counters are updated atomically, so I assume the reason
>>>> for
>>>> delaying the updates is that atomics are expensive. But I could not
>>>> find a
>>>> good reason why the counters are updated atomically. To me, all
>>>> modifications seem happen under lock protection
>>>> (SpaceManager::expand_lock()). What am I overlooking?
>>>>
>>>>
>>> I don't think you are overlooking anything. The fact that these are
>>> updated with atomics is something that I've noticed as well at some point
>>> but I don't think I ever got around to fixing that.
>>>
>>>
>>> I'm not sure I understand where in the code the delayed and accumulated
>>> updates take place but if you think that's the case then it's probably
>>> true. I suspect that at this point you are one of the handful of people
>>> who
>>> are familiar with the chunk allocation code :)
>>>
>>>
>>> :) That is good news, because I then can straighten the updates out, this
>> makes making the changes much simpler. Will probably do this in a separate
>> fix. Thank you!
>>
>
> I came to the same conclusion as Mikael the last time I checked, but due
> to lack of time I didn't got around to fix it. Please send this out as a
> separate patch, it will make reviewing much easier.
>
> Thanks,
> Erik
>
>
Thanks, Erik, will do that.


>
> Thomas
>>
>>
>> Regards
>>> /Mikael
>>>
>>>
>>>
>>> Thanks a lot,
>>>>
>>>> Kind Regards, Thomas
>>>>
>>>>
>>>>