RFR(XS): 8178497: Bug in MutableNUMASpace::ensure_parsability

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(XS): 8178497: Bug in MutableNUMASpace::ensure_parsability

sangheon.kim@oracle.com
Hi all,

Can I have some reviews for this tiny pointer arithmetic change?
Current code doesn't use pointer arithmetic, so the the resulting values
are wrong(too small). i.e. adding two values first and then typecast to
HeapWord* which is wrong here.
e.g.
intptr_t cur_top = X;
size_t touched_words = XX;
...
align_down((HeapWord*)(cur_top + touched_words), XXX);

This should be 'align_down( (HeapWord*)cur_top + touched_words, XXXX);'.

As I don't see any good reason of using 'intptr_t', changed to use
'HeapWord*' and changed related stuff.

I didn't add regression test or some further investigation as this is
clear that the calculation is wrong. And hard to provoke the problem
outside.

CR: https://bugs.openjdk.java.net/browse/JDK-8178497
Webrev: http://cr.openjdk.java.net/~sangheki/8178497/webrev.0
Testing: local building

Thanks,
Sangheon
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8178497: Bug in MutableNUMASpace::ensure_parsability

Thomas Schatzl
Hi Sangheon,

On Mon, 2017-11-20 at 11:05 -0800, sangheon.kim wrote:

> Hi all,
>
> Can I have some reviews for this tiny pointer arithmetic change?
> Current code doesn't use pointer arithmetic, so the the resulting
> values are wrong(too small). i.e. adding two values first and then
> typecast to HeapWord* which is wrong here.
> e.g.
> intptr_t cur_top = X;
> size_t touched_words = XX;
> ...
> align_down((HeapWord*)(cur_top + touched_words), XXX);
>
> This should be 'align_down( (HeapWord*)cur_top + touched_words,
> XXXX);'.
>
> As I don't see any good reason of using 'intptr_t', changed to use
> 'HeapWord*' and changed related stuff.
>
> I didn't add regression test or some further investigation as this
> is clear that the calculation is wrong. And hard to provoke the
> problem outside.
>
> CR: https://bugs.openjdk.java.net/browse/JDK-8178497
> Webrev: http://cr.openjdk.java.net/~sangheki/8178497/webrev.0
> Testing: local building

  actually after reading this code a bit in more detail I think the
failure mode is "only" performance loss. I.e. the "invalid" part of a
MutableNUMASpace is freed and reallocated for better NUMA locality.

With a too small invalid size what happens is that this locality
improvement will not happen for that part of the MutableNUMASpace. I do
not think there is a useful way of creating a reproducer.

Looks good.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8178497: Bug in MutableNUMASpace::ensure_parsability

Stefan Johansson
In reply to this post by sangheon.kim@oracle.com
Hi Sangheon,

On 2017-11-20 20:05, sangheon.kim wrote:

> Hi all,
>
> Can I have some reviews for this tiny pointer arithmetic change?
> Current code doesn't use pointer arithmetic, so the the resulting
> values are wrong(too small). i.e. adding two values first and then
> typecast to HeapWord* which is wrong here.
> e.g.
> intptr_t cur_top = X;
> size_t touched_words = XX;
> ...
> align_down((HeapWord*)(cur_top + touched_words), XXX);
>
> This should be 'align_down( (HeapWord*)cur_top + touched_words, XXXX);'.
>
> As I don't see any good reason of using 'intptr_t', changed to use
> 'HeapWord*' and changed related stuff.
>
> I didn't add regression test or some further investigation as this is
> clear that the calculation is wrong. And hard to provoke the problem
> outside.
>
> CR: https://bugs.openjdk.java.net/browse/JDK-8178497
> Webrev: http://cr.openjdk.java.net/~sangheki/8178497/webrev.0
Thanks for taking care of this. I think the changes is good, but I would
feel even more certain if you ran some testing with NUMA and Parallel to
make sure we haven't overlooked anything.

Thanks,
StefanJ
> Testing: local building
>
> Thanks,
> Sangheon

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8178497: Bug in MutableNUMASpace::ensure_parsability

sangheon.kim@oracle.com
In reply to this post by Thomas Schatzl
Hi Thomas,

On 11/21/2017 03:27 AM, Thomas Schatzl wrote:

> Hi Sangheon,
>
> On Mon, 2017-11-20 at 11:05 -0800, sangheon.kim wrote:
>> Hi all,
>>
>> Can I have some reviews for this tiny pointer arithmetic change?
>> Current code doesn't use pointer arithmetic, so the the resulting
>> values are wrong(too small). i.e. adding two values first and then
>> typecast to HeapWord* which is wrong here.
>> e.g.
>> intptr_t cur_top = X;
>> size_t touched_words = XX;
>> ...
>> align_down((HeapWord*)(cur_top + touched_words), XXX);
>>
>> This should be 'align_down( (HeapWord*)cur_top + touched_words,
>> XXXX);'.
>>
>> As I don't see any good reason of using 'intptr_t', changed to use
>> 'HeapWord*' and changed related stuff.
>>
>> I didn't add regression test or some further investigation as this
>> is clear that the calculation is wrong. And hard to provoke the
>> problem outside.
>>
>> CR: https://bugs.openjdk.java.net/browse/JDK-8178497
>> Webrev: http://cr.openjdk.java.net/~sangheki/8178497/webrev.0
>> Testing: local building
>    actually after reading this code a bit in more detail I think the
> failure mode is "only" performance loss. I.e. the "invalid" part of a
> MutableNUMASpace is freed and reallocated for better NUMA locality.
Agree.

>
> With a too small invalid size what happens is that this locality
> improvement will not happen for that part of the MutableNUMASpace. I do
> not think there is a useful way of creating a reproducer.
>
> Looks good.
Thanks for the review.

Thanks,
Sangheon


>
> Thanks,
>    Thomas
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8178497: Bug in MutableNUMASpace::ensure_parsability

sangheon.kim@oracle.com
In reply to this post by Stefan Johansson
Hi Stefan,

Thank you for reviewing this.

On 11/21/2017 05:29 AM, Stefan Johansson wrote:

> Hi Sangheon,
>
> On 2017-11-20 20:05, sangheon.kim wrote:
>> Hi all,
>>
>> Can I have some reviews for this tiny pointer arithmetic change?
>> Current code doesn't use pointer arithmetic, so the the resulting
>> values are wrong(too small). i.e. adding two values first and then
>> typecast to HeapWord* which is wrong here.
>> e.g.
>> intptr_t cur_top = X;
>> size_t touched_words = XX;
>> ...
>> align_down((HeapWord*)(cur_top + touched_words), XXX);
>>
>> This should be 'align_down( (HeapWord*)cur_top + touched_words, XXXX);'.
>>
>> As I don't see any good reason of using 'intptr_t', changed to use
>> 'HeapWord*' and changed related stuff.
>>
>> I didn't add regression test or some further investigation as this is
>> clear that the calculation is wrong. And hard to provoke the problem
>> outside.
>>
>> CR: https://bugs.openjdk.java.net/browse/JDK-8178497
>> Webrev: http://cr.openjdk.java.net/~sangheki/8178497/webrev.0
> Thanks for taking care of this. I think the changes is good, but I
> would feel even more certain if you ran some testing with NUMA and
> Parallel to make sure we haven't overlooked anything.
Okay, I added some explanation about what would happen.
And also ran hundred runs of GCOld with NUMA enabled on Solaris, of
course no problem.

Hope this is sufficient to go. :)

Thanks,
Sangheon


>
> Thanks,
> StefanJ
>> Testing: local building
>>
>> Thanks,
>> Sangheon
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS): 8178497: Bug in MutableNUMASpace::ensure_parsability

Stefan Johansson


On 2017-11-22 22:23, sangheon.kim wrote:

> Hi Stefan,
>
> Thank you for reviewing this.
>
> On 11/21/2017 05:29 AM, Stefan Johansson wrote:
>> Hi Sangheon,
>>
>> On 2017-11-20 20:05, sangheon.kim wrote:
>>> Hi all,
>>>
>>> Can I have some reviews for this tiny pointer arithmetic change?
>>> Current code doesn't use pointer arithmetic, so the the resulting
>>> values are wrong(too small). i.e. adding two values first and then
>>> typecast to HeapWord* which is wrong here.
>>> e.g.
>>> intptr_t cur_top = X;
>>> size_t touched_words = XX;
>>> ...
>>> align_down((HeapWord*)(cur_top + touched_words), XXX);
>>>
>>> This should be 'align_down( (HeapWord*)cur_top + touched_words,
>>> XXXX);'.
>>>
>>> As I don't see any good reason of using 'intptr_t', changed to use
>>> 'HeapWord*' and changed related stuff.
>>>
>>> I didn't add regression test or some further investigation as this
>>> is clear that the calculation is wrong. And hard to provoke the
>>> problem outside.
>>>
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8178497
>>> Webrev: http://cr.openjdk.java.net/~sangheki/8178497/webrev.0
>> Thanks for taking care of this. I think the changes is good, but I
>> would feel even more certain if you ran some testing with NUMA and
>> Parallel to make sure we haven't overlooked anything.
> Okay, I added some explanation about what would happen.
> And also ran hundred runs of GCOld with NUMA enabled on Solaris, of
> course no problem.
>
> Hope this is sufficient to go. :)
>
Thanks Sangheon,

That's great.

Cheers,
Stefan

> Thanks,
> Sangheon
>
>
>>
>> Thanks,
>> StefanJ
>>> Testing: local building
>>>
>>> Thanks,
>>> Sangheon
>>
>