RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion

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

RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion

Yasumasa Suenaga-4
Hi all,

G1HeapRegionTable#getByAddress() returns incorrect HeapRegion which contains incorrect address. We can see it in Stack Memory window on HSDB. Some oop addresses are shown as Free Region (attached image).

G1HeapRegion#getByAddress() should create HeapRegion instance from the address in _biased_base array.

I uploaded webrev. Could you review it?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.00/

I've tested this change with test/hotspot/jtreg/serviceability/sa, it works fine.
But I received some failure from Mach 5. I also tested this change via submit repos.

   http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171228-0605-8272

I cannot access this URL. Could you share the result?
Also I cannot access JPRT. So I need a sponsor.


Thanks,

Yasumasa
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion

David Holmes
Hi Yasumasa,

Not a review ...

On 29/12/2017 11:16 PM, Yasumasa Suenaga wrote:

> Hi all,
>
> G1HeapRegionTable#getByAddress() returns incorrect HeapRegion which
> contains incorrect address. We can see it in Stack Memory window on
> HSDB. Some oop addresses are shown as Free Region (attached image).
>
> G1HeapRegion#getByAddress() should create HeapRegion instance from the
> address in _biased_base array.
>
> I uploaded webrev. Could you review it?
>
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.00/
>
> I've tested this change with test/hotspot/jtreg/serviceability/sa, it
> works fine.
> But I received some failure from Mach 5. I also tested this change via
> submit repos.
>
>    
> http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171228-0605-8272 
>
>
> I cannot access this URL. Could you share the result?

How did you submit to mach5 ???

Anyway the failure is with:

test/hotspot/jtreg/serviceability/sa/TestG1HeapRegion.java

On linux and OS X:

  stderr: [Exception in thread "main" java.lang.NullPointerException
        at
TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70)
        at
jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121)
        at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
        at TestG1HeapRegion.main(TestG1HeapRegion.java:129)

On Solaris sparcv9:

  stderr: [Exception in thread "main" java.lang.RuntimeException:
Address of HeapRegion does not match.: expected 0x00000007afb00000 to
equal 0x00000007afc00000
        at jdk.test.lib.Asserts.fail(Asserts.java:594)
        at jdk.test.lib.Asserts.assertEquals(Asserts.java:205)
        at
TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70)
        at
jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121)
        at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
        at TestG1HeapRegion.main(TestG1HeapRegion.java:129)
]

David
-----

> Also I cannot access JPRT. So I need a sponsor.
>
>
> Thanks,
>
> Yasumasa
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion

Yasumasa Suenaga-4
Hi David,


> How did you submit to mach5 ???

I'm using Submit Repo for testing:
   https://wiki.openjdk.java.net/display/Build/Submit+Repo


> Anyway the failure is with:

Thanks!
I've fixed them in new webrev:
   http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/

This webrev has passed Mach 5 tier 1 tests in Submit Repo:
   http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171231-0202-8291


Yasumasa


On 2017/12/30 10:31, David Holmes wrote:

> Hi Yasumasa,
>
> Not a review ...
>
> On 29/12/2017 11:16 PM, Yasumasa Suenaga wrote:
>> Hi all,
>>
>> G1HeapRegionTable#getByAddress() returns incorrect HeapRegion which contains incorrect address. We can see it in Stack Memory window on HSDB. Some oop addresses are shown as Free Region (attached image).
>>
>> G1HeapRegion#getByAddress() should create HeapRegion instance from the address in _biased_base array.
>>
>> I uploaded webrev. Could you review it?
>>
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.00/
>>
>> I've tested this change with test/hotspot/jtreg/serviceability/sa, it works fine.
>> But I received some failure from Mach 5. I also tested this change via submit repos.
>>
>> http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171228-0605-8272
>>
>> I cannot access this URL. Could you share the result?
>
> How did you submit to mach5 ???
>
> Anyway the failure is with:
>
> test/hotspot/jtreg/serviceability/sa/TestG1HeapRegion.java
>
> On linux and OS X:
>
>   stderr: [Exception in thread "main" java.lang.NullPointerException
>      at TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70)
>      at jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121)
>      at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
>      at TestG1HeapRegion.main(TestG1HeapRegion.java:129)
>
> On Solaris sparcv9:
>
>   stderr: [Exception in thread "main" java.lang.RuntimeException: Address of HeapRegion does not match.: expected 0x00000007afb00000 to equal 0x00000007afc00000
>      at jdk.test.lib.Asserts.fail(Asserts.java:594)
>      at jdk.test.lib.Asserts.assertEquals(Asserts.java:205)
>      at TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70)
>      at jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121)
>      at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
>      at TestG1HeapRegion.main(TestG1HeapRegion.java:129)
> ]
>
> David
> -----
>
>> Also I cannot access JPRT. So I need a sponsor.
>>
>>
>> Thanks,
>>
>> Yasumasa
Reply | Threaded
Open this post in threaded view
|

PING: RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion

Yasumasa Suenaga-4
PING:
Could you review and sponsor it?

>    http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/


Yasumasa


On 2017/12/31 13:33, Yasumasa Suenaga wrote:

> Hi David,
>
>
>> How did you submit to mach5 ???
>
> I'm using Submit Repo for testing:
>    https://wiki.openjdk.java.net/display/Build/Submit+Repo
>
>
>> Anyway the failure is with:
>
> Thanks!
> I've fixed them in new webrev:
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/
>
> This webrev has passed Mach 5 tier 1 tests in Submit Repo:
>    http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171231-0202-8291
>
>
> Yasumasa
>
>
> On 2017/12/30 10:31, David Holmes wrote:
>> Hi Yasumasa,
>>
>> Not a review ...
>>
>> On 29/12/2017 11:16 PM, Yasumasa Suenaga wrote:
>>> Hi all,
>>>
>>> G1HeapRegionTable#getByAddress() returns incorrect HeapRegion which contains incorrect address. We can see it in Stack Memory window on HSDB. Some oop addresses are shown as Free Region (attached image).
>>>
>>> G1HeapRegion#getByAddress() should create HeapRegion instance from the address in _biased_base array.
>>>
>>> I uploaded webrev. Could you review it?
>>>
>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.00/
>>>
>>> I've tested this change with test/hotspot/jtreg/serviceability/sa, it works fine.
>>> But I received some failure from Mach 5. I also tested this change via submit repos.
>>>
>>> http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171228-0605-8272
>>>
>>> I cannot access this URL. Could you share the result?
>>
>> How did you submit to mach5 ???
>>
>> Anyway the failure is with:
>>
>> test/hotspot/jtreg/serviceability/sa/TestG1HeapRegion.java
>>
>> On linux and OS X:
>>
>>   stderr: [Exception in thread "main" java.lang.NullPointerException
>>      at TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70)
>>      at jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121)
>>      at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
>>      at TestG1HeapRegion.main(TestG1HeapRegion.java:129)
>>
>> On Solaris sparcv9:
>>
>>   stderr: [Exception in thread "main" java.lang.RuntimeException: Address of HeapRegion does not match.: expected 0x00000007afb00000 to equal 0x00000007afc00000
>>      at jdk.test.lib.Asserts.fail(Asserts.java:594)
>>      at jdk.test.lib.Asserts.assertEquals(Asserts.java:205)
>>      at TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70)
>>      at jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121)
>>      at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
>>      at TestG1HeapRegion.main(TestG1HeapRegion.java:129)
>> ]
>>
>> David
>> -----
>>
>>> Also I cannot access JPRT. So I need a sponsor.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion

Jini George
In reply to this post by Yasumasa Suenaga-4
Hi Yasumasa,

The changes look good to me. Please do update the copyright year to 2018.

Thanks!
Jini (Not a Reviewer).



On 12/31/2017 10:03 AM, Yasumasa Suenaga wrote:

> Hi David,
>
>
>> How did you submit to mach5 ???
>
> I'm using Submit Repo for testing:
>    https://wiki.openjdk.java.net/display/Build/Submit+Repo
>
>
>> Anyway the failure is with:
>
> Thanks!
> I've fixed them in new webrev:
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/
>
> This webrev has passed Mach 5 tier 1 tests in Submit Repo:
>    
> http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171231-0202-8291 
>
>
>
> Yasumasa
>
>
> On 2017/12/30 10:31, David Holmes wrote:
>> Hi Yasumasa,
>>
>> Not a review ...
>>
>> On 29/12/2017 11:16 PM, Yasumasa Suenaga wrote:
>>> Hi all,
>>>
>>> G1HeapRegionTable#getByAddress() returns incorrect HeapRegion which
>>> contains incorrect address. We can see it in Stack Memory window on
>>> HSDB. Some oop addresses are shown as Free Region (attached image).
>>>
>>> G1HeapRegion#getByAddress() should create HeapRegion instance from
>>> the address in _biased_base array.
>>>
>>> I uploaded webrev. Could you review it?
>>>
>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.00/
>>>
>>> I've tested this change with test/hotspot/jtreg/serviceability/sa, it
>>> works fine.
>>> But I received some failure from Mach 5. I also tested this change
>>> via submit repos.
>>>
>>> http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171228-0605-8272 
>>>
>>>
>>> I cannot access this URL. Could you share the result?
>>
>> How did you submit to mach5 ???
>>
>> Anyway the failure is with:
>>
>> test/hotspot/jtreg/serviceability/sa/TestG1HeapRegion.java
>>
>> On linux and OS X:
>>
>>   stderr: [Exception in thread "main" java.lang.NullPointerException
>>      at
>> TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70)
>>
>>      at
>> jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121)
>>
>>      at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
>>      at TestG1HeapRegion.main(TestG1HeapRegion.java:129)
>>
>> On Solaris sparcv9:
>>
>>   stderr: [Exception in thread "main" java.lang.RuntimeException:
>> Address of HeapRegion does not match.: expected 0x00000007afb00000 to
>> equal 0x00000007afc00000
>>      at jdk.test.lib.Asserts.fail(Asserts.java:594)
>>      at jdk.test.lib.Asserts.assertEquals(Asserts.java:205)
>>      at
>> TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70)
>>
>>      at
>> jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121)
>>
>>      at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
>>      at TestG1HeapRegion.main(TestG1HeapRegion.java:129)
>> ]
>>
>> David
>> -----
>>
>>> Also I cannot access JPRT. So I need a sponsor.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion

Yasumasa Suenaga-4
Hi Jini,

Thank you for your review!
I will update the copyright year in this changeset.

I'm waiting for Reviewer and sponsor.


Yasumasa


On 2018/01/22 13:14, Jini George wrote:

> Hi Yasumasa,
>
> The changes look good to me. Please do update the copyright year to 2018.
>
> Thanks!
> Jini (Not a Reviewer).
>
>
>
> On 12/31/2017 10:03 AM, Yasumasa Suenaga wrote:
>> Hi David,
>>
>>
>>> How did you submit to mach5 ???
>>
>> I'm using Submit Repo for testing:
>>    https://wiki.openjdk.java.net/display/Build/Submit+Repo
>>
>>
>>> Anyway the failure is with:
>>
>> Thanks!
>> I've fixed them in new webrev:
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/
>>
>> This webrev has passed Mach 5 tier 1 tests in Submit Repo:
>> http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171231-0202-8291
>>
>>
>> Yasumasa
>>
>>
>> On 2017/12/30 10:31, David Holmes wrote:
>>> Hi Yasumasa,
>>>
>>> Not a review ...
>>>
>>> On 29/12/2017 11:16 PM, Yasumasa Suenaga wrote:
>>>> Hi all,
>>>>
>>>> G1HeapRegionTable#getByAddress() returns incorrect HeapRegion which contains incorrect address. We can see it in Stack Memory window on HSDB. Some oop addresses are shown as Free Region (attached image).
>>>>
>>>> G1HeapRegion#getByAddress() should create HeapRegion instance from the address in _biased_base array.
>>>>
>>>> I uploaded webrev. Could you review it?
>>>>
>>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.00/
>>>>
>>>> I've tested this change with test/hotspot/jtreg/serviceability/sa, it works fine.
>>>> But I received some failure from Mach 5. I also tested this change via submit repos.
>>>>
>>>> http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171228-0605-8272
>>>>
>>>> I cannot access this URL. Could you share the result?
>>>
>>> How did you submit to mach5 ???
>>>
>>> Anyway the failure is with:
>>>
>>> test/hotspot/jtreg/serviceability/sa/TestG1HeapRegion.java
>>>
>>> On linux and OS X:
>>>
>>>   stderr: [Exception in thread "main" java.lang.NullPointerException
>>>      at TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70)
>>>      at jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121)
>>>      at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
>>>      at TestG1HeapRegion.main(TestG1HeapRegion.java:129)
>>>
>>> On Solaris sparcv9:
>>>
>>>   stderr: [Exception in thread "main" java.lang.RuntimeException: Address of HeapRegion does not match.: expected 0x00000007afb00000 to equal 0x00000007afc00000
>>>      at jdk.test.lib.Asserts.fail(Asserts.java:594)
>>>      at jdk.test.lib.Asserts.assertEquals(Asserts.java:205)
>>>      at TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70)
>>>      at jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121)
>>>      at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
>>>      at TestG1HeapRegion.main(TestG1HeapRegion.java:129)
>>> ]
>>>
>>> David
>>> -----
>>>
>>>> Also I cannot access JPRT. So I need a sponsor.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
Reply | Threaded
Open this post in threaded view
|

PING: RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion

Yasumasa Suenaga-4
PING: Could you review it?

>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/

This webrev has been reviewed by Jini.
I need a Reviewer and sponsor.


Yasumasa


On 2018/01/22 19:53, Yasumasa Suenaga wrote:

> Hi Jini,
>
> Thank you for your review!
> I will update the copyright year in this changeset.
>
> I'm waiting for Reviewer and sponsor.
>
>
> Yasumasa
>
>
> On 2018/01/22 13:14, Jini George wrote:
>> Hi Yasumasa,
>>
>> The changes look good to me. Please do update the copyright year to 2018.
>>
>> Thanks!
>> Jini (Not a Reviewer).
>>
>>
>>
>> On 12/31/2017 10:03 AM, Yasumasa Suenaga wrote:
>>> Hi David,
>>>
>>>
>>>> How did you submit to mach5 ???
>>>
>>> I'm using Submit Repo for testing:
>>>    https://wiki.openjdk.java.net/display/Build/Submit+Repo
>>>
>>>
>>>> Anyway the failure is with:
>>>
>>> Thanks!
>>> I've fixed them in new webrev:
>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/
>>>
>>> This webrev has passed Mach 5 tier 1 tests in Submit Repo:
>>> http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171231-0202-8291
>>>
>>>
>>> Yasumasa
>>>
>>>
>>> On 2017/12/30 10:31, David Holmes wrote:
>>>> Hi Yasumasa,
>>>>
>>>> Not a review ...
>>>>
>>>> On 29/12/2017 11:16 PM, Yasumasa Suenaga wrote:
>>>>> Hi all,
>>>>>
>>>>> G1HeapRegionTable#getByAddress() returns incorrect HeapRegion which contains incorrect address. We can see it in Stack Memory window on HSDB. Some oop addresses are shown as Free Region (attached image).
>>>>>
>>>>> G1HeapRegion#getByAddress() should create HeapRegion instance from the address in _biased_base array.
>>>>>
>>>>> I uploaded webrev. Could you review it?
>>>>>
>>>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.00/
>>>>>
>>>>> I've tested this change with test/hotspot/jtreg/serviceability/sa, it works fine.
>>>>> But I received some failure from Mach 5. I also tested this change via submit repos.
>>>>>
>>>>> http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171228-0605-8272
>>>>>
>>>>> I cannot access this URL. Could you share the result?
>>>>
>>>> How did you submit to mach5 ???
>>>>
>>>> Anyway the failure is with:
>>>>
>>>> test/hotspot/jtreg/serviceability/sa/TestG1HeapRegion.java
>>>>
>>>> On linux and OS X:
>>>>
>>>>   stderr: [Exception in thread "main" java.lang.NullPointerException
>>>>      at TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70)
>>>>      at jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121)
>>>>      at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
>>>>      at TestG1HeapRegion.main(TestG1HeapRegion.java:129)
>>>>
>>>> On Solaris sparcv9:
>>>>
>>>>   stderr: [Exception in thread "main" java.lang.RuntimeException: Address of HeapRegion does not match.: expected 0x00000007afb00000 to equal 0x00000007afc00000
>>>>      at jdk.test.lib.Asserts.fail(Asserts.java:594)
>>>>      at jdk.test.lib.Asserts.assertEquals(Asserts.java:205)
>>>>      at TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70)
>>>>      at jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121)
>>>>      at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
>>>>      at TestG1HeapRegion.main(TestG1HeapRegion.java:129)
>>>> ]
>>>>
>>>> David
>>>> -----
>>>>
>>>>> Also I cannot access JPRT. So I need a sponsor.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
Reply | Threaded
Open this post in threaded view
|

Re: PING: RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion

Thomas Schatzl
Hi,

On Tue, 2018-01-30 at 07:00 +1000, David Holmes wrote:

> Added in hotspot-gc-dev. Although this is in the SA it is about the
> SA interaction with G1 and so likely needs someone familiar with G1
> to review it.
>
> David
>
> On 28/01/2018 10:41 PM, Yasumasa Suenaga wrote:
> > PING: Could you review it?
> >
> > > >  http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/
> >
> > This webrev has been reviewed by Jini.
> > I need a Reviewer and sponsor.

  looks good to me - however there is another (pre-existing) bug: the
shift in that code should be a logical shift, not an arithmetic shift.

I.e. ">>" instead of ">>>".

I will run the patch through testing and report back in a few hours.
Should be okay.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: PING: RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion

Thomas Schatzl
Hi all,

On Tue, 2018-01-30 at 11:06 +0100, Thomas Schatzl wrote:

> Hi,
>
> On Tue, 2018-01-30 at 07:00 +1000, David Holmes wrote:
> > Added in hotspot-gc-dev. Although this is in the SA it is about the
> > SA interaction with G1 and so likely needs someone familiar with G1
> > to review it.
> >
> > David
> >
> > On 28/01/2018 10:41 PM, Yasumasa Suenaga wrote:
> > > PING: Could you review it?
> > >
> > > > >  http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/
> > >
> > > This webrev has been reviewed by Jini.
> > > I need a Reviewer and sponsor.
>
>   looks good to me - however there is another (pre-existing) bug: the
> shift in that code should be a logical shift, not an arithmetic
> shift.
>
> I.e. ">>" instead of ">>>".
>
> I will run the patch through testing and report back in a few hours.
> Should be okay.

  is good. Do you want to fix the issue with the shift operator too
here, or use another CR?

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: PING: RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion

Yasumasa Suenaga-4
Hi Thomas,

>>   looks good to me - however there is another (pre-existing) bug: the
>> shift in that code should be a logical shift, not an arithmetic
>> shift.
>>
>> I.e. ">>" instead of ">>>".
>>
>> I will run the patch through testing and report back in a few hours.
>> Should be okay.
>
>   is good. Do you want to fix the issue with the shift operator too
> here, or use another CR?

Thanks!

If the use of ">>>" is the bug, I want to fix it in new bug ticket.
I do not think the use of ">>>" is not a bug.

I g1BiasedArray.hpp, G1BiasedMappedArray::get_by_address() uses ">>"
operator to calculate biased_index:
  http://hg.openjdk.java.net/jdk/hs/file/ee513596f3ee/src/hotspot/share/gc/g1/g1BiasedArray.hpp#l134

idx_t is defined as size_t. So it is calculated as unsigned value.
In JLS 15.19, ">>>" is for unsigned. If we use ">>", it might remain
MSB in some cases.
  https://docs.oracle.com/javase/specs/jls/se9/html/jls-15.html#jls-15.19

Thus I think this is not a bug.


Thanks,

Yasumasa


2018-01-31 0:51 GMT+09:00 Thomas Schatzl <[hidden email]>:

> Hi all,
>
> On Tue, 2018-01-30 at 11:06 +0100, Thomas Schatzl wrote:
>> Hi,
>>
>> On Tue, 2018-01-30 at 07:00 +1000, David Holmes wrote:
>> > Added in hotspot-gc-dev. Although this is in the SA it is about the
>> > SA interaction with G1 and so likely needs someone familiar with G1
>> > to review it.
>> >
>> > David
>> >
>> > On 28/01/2018 10:41 PM, Yasumasa Suenaga wrote:
>> > > PING: Could you review it?
>> > >
>> > > > >  http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/
>> > >
>> > > This webrev has been reviewed by Jini.
>> > > I need a Reviewer and sponsor.
>>
>>   looks good to me - however there is another (pre-existing) bug: the
>> shift in that code should be a logical shift, not an arithmetic
>> shift.
>>
>> I.e. ">>" instead of ">>>".
>>
>> I will run the patch through testing and report back in a few hours.
>> Should be okay.
>
>   is good. Do you want to fix the issue with the shift operator too
> here, or use another CR?
>
> Thanks,
>   Thomas
>