OpenJDK › OpenJDK Hotspot › OpenJDK Hotspot Compiler Development List

Re: RFR: 8264223: CodeHeap::verify fails extra_hops assertion in fastdebug test [v3]

Classic

List

Threaded

7 messages Options

Lutz Schmidt

Re: RFR: 8264223: CodeHeap::verify fails extra_hops assertion in fastdebug test [v3]

On Fri, 26 Mar 2021 08:28:40 GMT, Hui Shi <[hidden email]> wrote:

>> When test with -XX:+VerifyCodeCache, many tests fail due to extra_hops assertion in CodeHeap::verify. See full dump in JBS.
>>
>> # Internal Error (src/hotspot/share/memory/heap.cpp:838), pid=1525697, tid=1525715
>> # assert((count == 0) || (extra_hops < (16 + 2*count))) failed: CodeHeap: many extra hops due to optimization. blocks: 234, extra hops: 484.
>> Discussion in https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-October/035508.html doesn't tell where assertion (extra_hops < (16 + 2*count) comes from.
>>
>> In CodeHeap::mark_segmap_as_used wehn is_FreeBlock_join is true, it creates extra hops in _segmap. _fragmentation_count in inced and trigger defrag_segmap when reach threshold. In my understanding, extra hop can not guarantee under (16 + 2*count).
>>
>> In following extreme case, before HeapBlock free, segmap is all 0 (each blob use 1 smallest segment), suppose free action is applied from right to left. This increase 9 unnecessary hop for 1 continous HeapBlock. assertion (extra_hops < (16 + 2*count) is not safe.
>> |0|0|0|0|0|0|0|0|0|0|
>> after free, it will be
>> |0|1|1|1|1|1|1|1|1|1|
>>
>> Proposed fix is assert extra hops not exceed _fragmentation_count. And if it exceeds (16 + 2 * count), give warning on two many extra hops.
>>
>> fastdebug tier1, tier2 with VerifyCodeCache passed on X86_64 linux, no extra assertion found.
>
> Hui Shi has updated the pull request incrementally with one additional commit since the last revision:
>
> update copyright year

The change looks good to me.
Thank you for digging into the tricky code to understand what's happening. The previous limit (16+2*count) was purely heuristic. Obviously, you have a more pathological test case.

-------------

Marked as reviewed by lucy (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3212

Lutz Schmidt

Re: RFR: 8264223: CodeHeap::verify fails extra_hops assertion in fastdebug test [v3]

On Wed, 31 Mar 2021 12:50:37 GMT, Lutz Schmidt <[hidden email]> wrote:

>> Hui Shi has updated the pull request incrementally with one additional commit since the last revision:
>>
>> update copyright year
>
> The change looks good to me.
> Thank you for digging into the tricky code to understand what's happening. The previous limit (16+2*count) was purely heuristic. Obviously, you have a more pathological test case.

I would volunteer to sponsor this PR

-------------

PR: https://git.openjdk.java.net/jdk/pull/3212

Hui Shi-2

Re: RFR: 8264223: CodeHeap::verify fails extra_hops assertion in fastdebug test [v3]

In reply to this post by Lutz Schmidt

On Wed, 31 Mar 2021 12:50:37 GMT, Lutz Schmidt <[hidden email]> wrote:

> The change looks good to me.
> Thank you for digging into the tricky code to understand what's happening. The previous limit (16+2*count) was purely heuristic. Obviously, you have a more pathological test case.

Thanks! The entire CodeHeap optimization is great!

-------------

PR: https://git.openjdk.java.net/jdk/pull/3212

Lutz Schmidt

Re: RFR: 8264223: CodeHeap::verify fails extra_hops assertion in fastdebug test [v3]

On Thu, 1 Apr 2021 01:01:19 GMT, Hui Shi <[hidden email]> wrote:

>> The change looks good to me.
>> Thank you for digging into the tricky code to understand what's happening. The previous limit (16+2*count) was purely heuristic. Obviously, you have a more pathological test case.
>
>> The change looks good to me.
>> Thank you for digging into the tricky code to understand what's happening. The previous limit (16+2*count) was purely heuristic. Obviously, you have a more pathological test case.
>
> Thanks! The entire CodeHeap optimization is great!

You will need a second review before the fix can be integrated.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3212

Aleksey Shipilev-5

Re: RFR: 8264223: CodeHeap::verify fails extra_hops assertion in fastdebug test [v3]

In reply to this post by Lutz Schmidt

On Fri, 26 Mar 2021 08:28:40 GMT, Hui Shi <[hidden email]> wrote:

It looks fine. But I have a question: are we reasonably sure that `extra_hops <= _fragmentation_count` always. More specifically, when `_fragmentation_count` drops to `0`, are `extra_hops` guaranteed to be `0` as well?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3212

Lutz Schmidt

Re: RFR: 8264223: CodeHeap::verify fails extra_hops assertion in fastdebug test [v3]

On Thu, 1 Apr 2021 08:01:16 GMT, Aleksey Shipilev <[hidden email]> wrote:

>> Hui Shi has updated the pull request incrementally with one additional commit since the last revision:
>>
>> update copyright year
>
> It looks fine. But I have a question: are we reasonably sure that `extra_hops <= _fragmentation_count` always. More specifically, when `_fragmentation_count` drops to `0`, are `extra_hops` guaranteed to be `0` as well?

@shipilev Short answer: yes.
Long answer:
_fragmentation_count is incremented every time the segment map becomes _potentially_ more fragmented by introducing an additional chunk, see mark_segmap_as_used(). Therefore, _fragmentation_count overestimates the actual segmap fragmentation.

Once _fragmentation_count hits the limit, defragmentation is triggered (defrag_segmap(true)) and the counter is set to zero. After defragmentation, segmap should not contain any extra hops - that's the purpose of defragmentation. If is does, I would classify that as a bug.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3212

Aleksey Shipilev-5

Re: RFR: 8264223: CodeHeap::verify fails extra_hops assertion in fastdebug test [v3]

In reply to this post by Lutz Schmidt

On Fri, 26 Mar 2021 08:28:40 GMT, Hui Shi <[hidden email]> wrote:

Okay then, thanks.

-------------

Marked as reviewed by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3212