Fwd: RFR(s-m): 8243535: NMT may show wrong numbers for CDS and CCS
Ioi Lam
ioi.lam at oracle.com
Mon Jun 8 17:18:31 UTC 2020
HI Thomas,
The CDS changes look good to me. On the NMT part, I took a cursory look
and didn't notice any problems, but I am not familiar with NMT to say
anything definitive.
One typo:
// They also will be tracked individually by NMT and can be tagged
with different flags.
// Note that this may cause the original space to loose its content.
loose -> lose
Thanks
- Ioi
On 6/8/20 5:14 AM, Thomas Stüfe wrote:
> ---------- Forwarded message ---------
> From: Zhengyu Gu <zgu at redhat.com>
> Date: Mon, Jun 8, 2020 at 2:11 PM
> Subject: Re: RFR(s-m): 8243535: NMT may show wrong numbers for CDS and CCS
> To: Thomas Stüfe <thomas.stuefe at gmail.com>, Ioi Lam <ioi.lam at oracle.com>
>
>
> Hi Thomas,
>
> NMT part looks good to me.
>
>
> Thanks,
>
> -Zhengyu
>
> On 6/8/20 4:43 AM, Thomas Stüfe wrote:
>> Hi Zhengyu and Ioi,
>>
>> would it be possible to get a review for this this week?
>>
>> I understand if this is not possible, but it would be nice to get this
>> into 15.
>>
>> Thanks, Thomas
>>
>>
>>
>> On Tue, May 26, 2020 at 8:38 AM Thomas Stüfe <thomas.stuefe at gmail.com
>> <mailto:thomas.stuefe at gmail.com>> wrote:
>>
>> Hi,
>>
>> please take a look at this fix:
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8243535?filter=-3
>> Webrev:
>>
> http://cr.openjdk.java.net/~stuefe/webrevs/8243535--nmt-does-not-handle-os--split_reserved_space()/webrev.00/webrev/
>> NMT is not able to correctly follow os::split_reserved_memory
>> operations. This causes misreports for cds and class space (see JBS
>> for details).
>>
>> The patch adds support to NMT for following split operations, and
>> fixes up the CDS side of things.
>>
>> Changes in NMT:
>>
>> - In virtualMemoryTracker.hpp/cpp, we now have a new
>> operation "split_reserved_region()". This one takes a reserved
>> region in NMT and splits it in two adjacent regions while keeping
>> the MEMFLAG tags and the original call stack intact.
>>
>> - For that to work I needed to officially abandon the notion that we
>> collapse adjacent regions in NMT. In practice, we did never do this
>> due to a coding error, but now we also do not want this. See
>> discussion with Zhengyu about this here:
>>
> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-May/039525.html
>> - As a small cleanup I also moved the implementation of
>> (Virtual|Committed)MemoryRegion::compare() and
>> (Virtual|Committed)MemoryRegion::equals() up to the base class,
>> since they were identical, and simplified compare() a bit.
>>
>> - In memTracker.hpp I just marshall the new call through like it is
>> done here
>>
>> - MemTracker::record_virtual_memory_split_reserved gets now called
>> from the Posix version of os::split_reserved_memory() to register
>> the split. On Windows, this is not necessary, since a split involves
>> releasing and re-reserving.
>>
>> ---
>>
>> Changes to/for CDS:
>>
>> CDS maps archive files into pre-reserved space (on Posix) and
>> mapping into reserved space is not handled by NMT on a general
>> level, that is why inside NMT there exists special logic to deal
>> with CDS archive file mappings. I left those alone.
>>
>> But I simplified/changed the logic of registration somewhat. In
>> FileMapInfo::map_region(), before, we would register the archive
>> spaces with NMT explicitly after mapping. This was done with two
>> separate calls to NMT: the first one for the case where we map into
>> pre-reserved space (which inside NMT actually is a noop since this
>> case is ignored, see above) and once for the case that we mapped
>> archive files into unreserved space (windows only).
>>
>> I needed a while to understand the logic. Then I simplified as
>> follows: I added a MEMFLAGS parameter to os::map_memory() which gets
>> uses when registering the mapping with NMT now. That way, no follow
>> up tag correction is needed, the mapping has the right tag right
>> away. This takes care of the case when we map into unreserved regions.
>>
>> In addition to this, in
>> MetaspaceShared::reserve_address_space_for_archives(), when
>> establishing the space for archive and class space, I now register
>> those two spaces with NMT after creating them.
>>
>> That makes for three different cases:
>>
>> -> case 1 (posix):
>> 1 we create the spaces
>> in MetaspaceShared::reserve_address_space_for_archives(). Register
>> them with NMT.
>> 2 in FileMapInfo::map_region(), we then map the archive files in
>> one by one using os::map_memory(). This will attempt to register
>> these regions, but inside NMT this case is ignored.
>>
>> -> case 2 (windows, first attempt)
>> 1 we create the spaces
>> in MetaspaceShared::reserve_address_space_for_archives(). Register
>> them with NMT.
>> 2 we then delete the archive space again to make space for
>> windows file mapping. This removes in NMT the registered address
>> range for the archive space.
>> 3 in FileMapInfo::map_region(), we then map the archive files in
>> one by one using os::map_memory(). This will register each region
>> separately with NMT.
>>
>> -> case 3 (windows, second attempt)
>> 1 we create the spaces
>> in MetaspaceShared::reserve_address_space_for_archives(). Register
>> them with NMT.
>> 2 in FileMapInfo::map_region(), we read the content of the
>> archive file sequentially. Nothing happens at NMT.
>>
>> If all this sounds complicated, it is less complicated than it was
>> before :)
>>
>> Note that since we do not merge adjacent regions, in detail report
>> the archive files may now appear as separate entities. I do not
>> really care, I even think this may be a feature. But if this is not
>> wanted, Zhengyu proposed to merge adjacent regions when displaying
>> them:
>>
> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-May/039534.html
> .
>> That would be a separate issue though.
>>
>> Tests ran at SAP CI tonight sucessfully.
>>
>> Thanks, Thomas
>>
>>
>>
>>
>>
>>
>>
>>
More information about the hotspot-runtime-dev
mailing list