RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly

2016-03-10 Thread Cheleswer Sahu 
Hi,

 

Please review the code changes for 
https://bugs.openjdk.java.net/browse/JDK-8151509.

 

Webrev link: http://cr.openjdk.java.net/~csahu/8151509/

 

Bug Brief: 

In check_addr0(),  pointer "p" is not updated correctly, because of this it was 
reading only first two entries from buffer.

 

 

Regards,

Cheleswer

Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly

2016-03-10 Thread Dmitry Dmitriev 

Hi Cheleswer,

Looks good, but I have questions/comments about other code in this function:
1) I think that "::close(fd);" should be inside "if (fd >= 0) {".
2) Just interesting, do you really need to set memory to 0 by memset?

Thanks,
Dmitry


On 10.03.2016 13:43, Cheleswer Sahu wrote:


Hi,

Please review the code changes for 
https://bugs.openjdk.java.net/browse/JDK-8151509.


Webrev link: http://cr.openjdk.java.net/~csahu/8151509/ 



Bug Brief:

In check_addr0(),  pointer ”p” is not updated correctly, because of 
this it was reading only first two entries from buffer.


Regards,

Cheleswer

Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly

2016-03-10 Thread Thomas Stüfe 
Hi Cheleswer,

thanks for fixing this.

Some more issues:

- 1866 char *mbuff = (char *) calloc(read_chunk, sizeof(prmap_t) + 1);

Why the "+1"? It is unnecessary and causes the allocation to be 200 bytes
larger than necessary.

- 1880 st->print("Warning: Address: " PTR_FORMAT ", Size: %dK,
",p->pr_vaddr, p->pr_size/1024);

Format specifier for Size is wrong.%d is int, but p->pr_size is size_t.
Theoretical truncation for mappings larger than 4g*1024.
(But I know this coding was there before)

Beside those points, I think both points of Dmitry are valid.

Also, I find


Kind Regards, Thomas

On Thu, Mar 10, 2016 at 1:45 PM, Dmitry Dmitriev  wrote:

> Hi Cheleswer,
>
> Looks good, but I have questions/comments about other code in this
> function:
> 1) I think that "::close(fd);" should be inside "if (fd >= 0) {".
> 2) Just interesting, do you really need to set memory to 0 by memset?
>
> Thanks,
> Dmitry
>
>
> On 10.03.2016 13:43, Cheleswer Sahu wrote:
>
>>
>> Hi,
>>
>> Please review the code changes for
>> https://bugs.openjdk.java.net/browse/JDK-8151509.
>>
>> Webrev link: http://cr.openjdk.java.net/~csahu/8151509/ <
>> http://cr.openjdk.java.net/%7Ecsahu/8151509/>
>>
>> Bug Brief:
>>
>> In check_addr0(),  pointer ”p” is not updated correctly, because of this
>> it was reading only first two entries from buffer.
>>
>> Regards,
>>
>> Cheleswer
>>
>>
>

Re: RFR[9u-dev]: 8151509: In check_addr0() function pointer is not updated correctly

2016-03-10 Thread Thomas Stüfe 
(Sorry, pressed Send button too early)

Just wanted to add that

1873   if( 0 != ret % sizeof(prmap_t)){
1874 break;
1875   }

may be a bit harsh, as it skips the entire mapping in case read() stopped
reading in a middle of a record. You could just continue to read until you
read the rest of the record.

Kind Regards, Thomas


On Thu, Mar 10, 2016 at 3:07 PM, Thomas Stüfe 
wrote:

> Hi Cheleswer,
>
> thanks for fixing this.
>
> Some more issues:
>
> - 1866 char *mbuff = (char *) calloc(read_chunk, sizeof(prmap_t) + 1);
>
> Why the "+1"? It is unnecessary and causes the allocation to be 200 bytes
> larger than necessary.
>
> - 1880 st->print("Warning: Address: " PTR_FORMAT ", Size: %dK,
> ",p->pr_vaddr, p->pr_size/1024);
>
> Format specifier for Size is wrong.%d is int, but p->pr_size is size_t.
> Theoretical truncation for mappings larger than 4g*1024.
> (But I know this coding was there before)
>
> Beside those points, I think both points of Dmitry are valid.
>
> Also, I find
>
>
> Kind Regards, Thomas
>
> On Thu, Mar 10, 2016 at 1:45 PM, Dmitry Dmitriev <
> dmitry.dmitr...@oracle.com> wrote:
>
>> Hi Cheleswer,
>>
>> Looks good, but I have questions/comments about other code in this
>> function:
>> 1) I think that "::close(fd);" should be inside "if (fd >= 0) {".
>> 2) Just interesting, do you really need to set memory to 0 by memset?
>>
>> Thanks,
>> Dmitry
>>
>>
>> On 10.03.2016 13:43, Cheleswer Sahu wrote:
>>
>>>
>>> Hi,
>>>
>>> Please review the code changes for
>>> https://bugs.openjdk.java.net/browse/JDK-8151509.
>>>
>>> Webrev link: http://cr.openjdk.java.net/~csahu/8151509/ <
>>> http://cr.openjdk.java.net/%7Ecsahu/8151509/>
>>>
>>> Bug Brief:
>>>
>>> In check_addr0(),  pointer ”p” is not updated correctly, because of this
>>> it was reading only first two entries from buffer.
>>>
>>> Regards,
>>>
>>> Cheleswer
>>>
>>>
>>
>

RFR: JDK-8151674: STW phases at Concurrent GC should count in PerfCounter

2016-03-10 Thread Yasumasa Suenaga 
Hi all,

This review request continues from:
  http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-March/016774.html
  http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-March/016896.html

I wonder that STW phases (Remark and Cleanup) at G1 are not counted in jstat 
FGC column. 
For example, Initial Mark and Remark at CMS are counted as FGC. 
We discussed on hotspot-gc-dev for this issue, and I proposed to add new 
PerfCounter
for CGC STW phases. 

I uploaded webrev. Could you review it?
hotspot: http://cr.openjdk.java.net/~ysuenaga/JDK-8151674/webrev.00/hotspot/
jdk: http://cr.openjdk.java.net/~ysuenaga/JDK-8151674/webrev.00/jdk/

For compatibility, this patch works the same as the current by default.
If you set -XX:+EnableConcGCPerfCounter, CGC counter will work fine.
(I want to set +EnableConcGCPerfCounter by default)


I cannot access JPRT.
So I need a sponsor.


Thanks,

Yasumasa