Serguei, Thank you for review. I'd updated webrev (see also below)
http://cr.openjdk.java.net/~dsamersoff/JDK-7127191/webrev.03/ On 2014-01-31 04:01, serguei.spit...@oracle.com wrote: > Hi Dmitry, > > > agent/src/os/linux/libproc_impl.c > > Q: To the lines 56-71 (I'm taking a blame for asking stupid questions): > Is it possible the name argument has no leading slash '/' ? > Does it work correctly in such a case? > Is there a need to add a slash after the alt_path? Linker always put full path to solib image to process .DYNSECTION. The only exclusion - couple of pseudo-objects providing interface to kernel like linux-gate.so etc, but SA doesn't handle it. I put appropriate comment. > agent/src/share/classes/sun/jvm/hotspot/debugger/linux/LinuxCDebugger.java > > Something is wrong with the comment as it is not clear: > > 59 /* Typically we don't have too much loaded object here, > 60 so final efforts to do a linear search less then sort and do > 61 binary serach */ > > Did you want to say: "too many objects" or "to match loaded object" ? > And what does it mean: > "so final efforts to do a linear search less then sort and do binary > serach" ? > > Did you want to say something like: > "do a linear search instead of binary search" Comments rephrased. > agent/src/share/classes/sun/jvm/hotspot/utilities/soql/sa.js > > nit: Remove redundant else at line 896, it saves the extra indent. I`m itching to reformat this function to make it readable since first time i saw it. You gives me enough courage to do it ;) So see webrev. > nit: More simplifications to consider: > > function closestSymbolFor(addr) { > var dso = (sa.cdbg == null) ? null : > sa.cdbg.loadObjectContainingPC(addr); > return (dso == null) ? null : dso.closestSymbolToPC(addr); > } > > function loadObjectContainingPC(addr) { > // sa.cdbg == null: no CDebugger support, return null > return (sa.cdbg == null) ? null : > sa.cdbg.loadObjectContainingPC(addr); > } I would prefer to keep full if for readability. -Dmitry > > > Thanks, > Serguei > > On 1/29/14 6:17 AM, Dmitry Samersoff wrote: >> Hi Everyone, >> >> Please review the fix. >> >> http://cr.openjdk.java.net/~dsamersoff/JDK-7127191/webrev.02/ >> >> -Dmitry >> >> On 2014-01-20 20:00, Dmitry Samersoff wrote: >>> Hi Everyone, >>> >>> Please review the fix. >>> >>> http://cr.openjdk.java.net/~dsamersoff/JDK-7127191/webrev.01/ >>> >>> This fix doesn't solve all problems with symbol lookup in SA, but >>> address the problem mentioned in bug reports. >>> >>> 1. I change algorithm of pathmap_open. Now, it tries to find library >>> hardly. >>> >>> 2. I decided not to fix broken binary search in loadObjectContainingPC, >>> because with less then 20 DSO's we typically have here performance of >>> just linear search is approx the same as load, sort, convert to array >>> and do binary search. >>> >>> -Dmitry >>> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
