RFR: 8196405: [REDO] NMT: add_committed_regions doesn't merge succeeding regions
Stefan Karlsson
stefan.karlsson at oracle.com
Thu Feb 15 17:26:37 UTC 2018
On 2018-02-15 17:46, coleen.phillimore at oracle.com wrote:
>
> Hi, This looks really good.
Thanks, Coleen.
>
> http://cr.openjdk.java.net/~stefank/8196405/webrev.01/src/hotspot/share/services/virtualMemoryTracker.cpp.udiff.html
>
>
> + // It would have made sense to use rgn->equals(...), but equals
> returns true for overlapping regions.
>
>
> Does anything call the Region<>::equals functions? I stumbled over
> this myself a while ago. Can they be removed with this patch?
Equals is used by the LinkedList implementation. However, I now see that
the SortedLinkedList subclass, that we are using, is overriding all
functions using equals. It probably would be possible to clean this up.
>
> Thank you for fixing this. The gtest looks great. There was
> another less comprehensive test in the NMT directory for this that
> hopefully you ran also with the runtime NMT tests.
Yes, I ran those tests as well.
Thanks,
StefanK
>
>
> Thanks,
> Coleen
>
> On 2/15/18 9:05 AM, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to fix the merging of committed regions in
>> NMT's VirtualMemoryTracker.
>>
>> http://cr.openjdk.java.net/~stefank/8196405/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8196405
>>
>> The previous attempt to fix this didn't deal with overlapping
>> committed regions correctly. This patch solves that case by first
>> removing regions, and parts of regions, that intersect with the newly
>> committed region. After that, the new region can be added and/or
>> merged with any adjacent regions.
>>
>> There used to be a special-case for code paths that both reserve and
>> commit memory at the same time. ReservedMemoryRegion had an
>> all_committed boolean to track that state, and subsequent commits in
>> the reserved region were ignored. There were a number of bugs cause
>> by this special-handeling, when regions were committed or uncommitted
>> inside an all_committed region. This patch removes the all_committed
>> property and instead queries the _committed_regions list where
>> necessary.
>>
>> The patch removes special case handling for mtThreadStacks. The
>> intention is that the add_committed_region and
>> remove_uncomitted_region functions should be correct, so that we
>> don't need that kind of special-case handling.
>>
>> The overlap_region function uses 'sz - 1', which would be broken if
>> sz ever was 0. I added asserts to check for this.
>>
>> I've added a small jtreg test for the initial problem that this bug
>> report was intending to fix. But I realized that the WhiteBox API
>> didn't support setting up and testing different call stacks for the
>> memory reservations and commits, so I introduced a C++ gtest. This
>> test is both faster to execute, easier to debug, and more flexible,
>> so I hope this is appreciated. :)
>>
>> I've tested this with the added unit tests, short run with
>> Kitchensink, and mach5 hs-tier1,hs-tier2. Anything else that I should
>> be running?
>>
>> While working on this I found two other bugs:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8198225
>> os::attempt_reserve_memory_at records memory as committed
>>
>> https://bugs.openjdk.java.net/browse/JDK-8198226
>> os::attempt_reserve_memory_at records reserved memory twice on windows
>>
>> Thanks,
>> StefanK
>
More information about the hotspot-runtime-dev
mailing list