RFR[9u-dev]: 8146683: check_addr0 should be more efficient

[Dmitry Samersoff]

Cheleswer,

Looks good for me.

-Dmitry


On 2016-03-03 19:28, Cheleswer Sahu wrote:
> Hi,
> 
> Please review the new code changes for this bug. I have removed  " fstat()"  call which seems not safe to read the "/proc/map/self" file and have made some other changes too. Please find the latest code in below link
> Webrev link: http://cr.openjdk.java.net/~csahu/8146683/webrev.01/
> 
> 
> Regards,
> Cheleswer
> 
> -----Original Message-----
> From: Dean Long 
> Sent: Wednesday, February 24, 2016 7:44 AM
> To: Daniel Daugherty; Thomas Stüfe; Cheleswer Sahu
> Cc: serviceability-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR[9u-dev]: 8146683: check_addr0 should be more efficient
> 
> On 2/23/2016 10:07 AM, Daniel D. Daugherty wrote:
>> On 2/23/16 5:56 AM, Thomas Stüfe wrote:
>>> Hi Cheleswer,
>>>
>>>
>>> On Tue, Feb 23, 2016 at 9:38 AM, Cheleswer Sahu 
>>> <cheleswer.sahu at oracle.com>
>>>   wrote:
>>>
>>>> Hi,
>>>>
>>>>
>>>>
>>>> Please review the code changes for
>>>> https://bugs.openjdk.java.net/browse/JDK-8146683 .
>>>>
>>>>
>>>>
>>>> Webrev link: http://cr.openjdk.java.net/~csahu/8146683/
>>>>
>>>> JBS Link: https://bugs.openjdk.java.net/browse/JDK-8146683
>>>>
>>>>
>>>>
>>>> Bug Brief:
>>>>
>>>> While investigating  bug
>>>> https://bugs.openjdk.java.net/browse/JDK-8144483
>>>> it has been observed that "check_addr0() " function  is not written 
>>>> efficiently.
>>>>
>>>> This function is trying to read each "prmap_t" entry in 
>>>> "/proc/self/map"
>>>> file one by one which is time taking and can cause in long pauses.
>>>>
>>>>
>>>>
>>>> Solution proposed:
>>>>
>>>> Instead of reading each "prmap_t" entry in "/proc/self/map" file one 
>>>> by one, read the entries in chunks. There can be many entries in "map"
>>>> file,
>>>> so I have decided to read these
>>>>
>>>> in chunks of 100  "prmap_t" entries. Reading entries in chunks saves 
>>>> a lot of read calls and results in smaller pause times.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Cheleswer
>>>>
>>>
>>> We saw cases, especially on Solaris, where the content of a file in the
>>> proc file system changed under us while we were reading it. So this 
>>> should
>>> be kept in mind. The original code seems also broken in that regard,
>>> because it assumes the ::read() call to read the full size of a prmap_t
>>> entry, but it may theoretically read an incomplete entry.
>>>
>>> As for your coding, you first estimate the size of the mapping file and
>>> then use this to determine how many entries to read; but when 
>>> reading, this
>>> information may already be stale. I think it would be more robust and 
>>> also
>>> simpler to just try to read as many entries as possible - in chunks, to
>>> make reading faster - until you get EOF.
>>>
>>> Kind Regards, Thomas
>>
>> I'm really sure that we've been down this road before. In particular,
>> Dmitry Samersoff has worked on this issue (or one very much like it)
>> before, but he is on vacation until the end of the month.
>>
> 
> There was a similar issue with Linux /proc/self/maps, and whether it was 
> safe to read the file with buffered stdio or not.  See 8009062 and 7017193.
> 
> dl
> 
>> Dan
>>
>>
>>>
>>> On Tue, Feb 23, 2016 at 9:38 AM, Cheleswer Sahu 
>>> <cheleswer.sahu at oracle.com>
>>> wrote:
>>>
>>>> Hi,
>>>>
>>>>
>>>>
>>>> Please review the code changes for
>>>> https://bugs.openjdk.java.net/browse/JDK-8146683 .
>>>>
>>>>
>>>>
>>>> Webrev link: http://cr.openjdk.java.net/~csahu/8146683/
>>>>
>>>> JBS Link: https://bugs.openjdk.java.net/browse/JDK-8146683
>>>>
>>>>
>>>>
>>>> Bug Brief:
>>>>
>>>> While investigating  bug 
>>>> https://bugs.openjdk.java.net/browse/JDK-8144483
>>>> it has been observed that "check_addr0() " function  is not written
>>>> efficiently.
>>>>
>>>> This function is trying to read each "prmap_t" entry in 
>>>> "/proc/self/map"
>>>> file one by one which is time taking and can cause in long pauses.
>>>>
>>>>
>>>>
>>>> Solution proposed:
>>>>
>>>> Instead of reading each "prmap_t" entry in "/proc/self/map" file one by
>>>> one, read the entries in chunks. There can be many entries in "map" 
>>>> file,
>>>> so I have decided to read these
>>>>
>>>> in chunks of 100  "prmap_t" entries. Reading entries in chunks saves 
>>>> a lot
>>>> of read calls and results in smaller pause times.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Cheleswer
>>>>
>>
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.