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@openjdk.java.net; hotspot-runtime-...@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.s...@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.s...@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.
