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

[Dean Long]

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
>>>
>