Hi Thomas,
Thanks for identifying the issue. I will file a new bug for this and correct the issue. Regards, Cheleswer From: Thomas Stüfe [mailto:thomas.stu...@gmail.com] Sent: Wednesday, March 09, 2016 4:44 PM To: David Holmes Cc: Kevin Walls; Cheleswer Sahu; Daniel Daugherty; serviceability-dev@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR[9u-dev]: 8146683: check_addr0 should be more efficient Hi all, I see the change is already pushed, but I am not sure this works as intended: We iterate now over all fully read items: + p = (prmap_t *)mbuff; + for(int i = 0; i < nmap; i++){ + if (p->pr_vaddr == 0x0) { + .... + } + p = (prmap_t *)(mbuff + sizeof(prmap_t)); + } Where do we move the pointer forward? The way I see it, we only ever examine the first and the second item, then repeat examining the second item over and over again. Instead of + p = (prmap_t *)(mbuff + sizeof(prmap_t)); Should this not be p = ((prmap_t *)mbuff) + i; or just p++; ? Kind Regards, Thomas On Mon, Mar 7, 2016 at 12:23 PM, David Holmes <HYPERLINK "mailto:david.hol...@oracle.com"david.hol...@oracle.com> wrote: On 7/03/2016 8:36 PM, Kevin Walls wrote: Hi Cheleswer, Looks good. (While we've discussed it offline I haven't actually offered a public review). Just one more thing, and this concerns the original code not the change: 1880 st->print("Warning: Address: 0x%x, Size: %dK, ",p->pr_vaddr, p->pr_size/1024, p->pr_mapname); Also %x is the wrong format specifier: typedef struct prmap { uintptr_t pr_vaddr; /* virtual address of mapping */ David ----- We pass 3 params when we only print two items? The "p->pr_mapname", we pass again on the next line where we do have the the "%s" in the format string, so this one is unnecessary. 8-) Thanks Kevin On 07/03/2016 10:07, Dmitry Samersoff wrote: 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: HYPERLINK "mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net; HYPERLINK "mailto:hotspot-runtime-...@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 <HYPERLINK "mailto:cheleswer.s...@oracle.com"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 <HYPERLINK "mailto:cheleswer.s...@oracle.com"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
