Dmitry,
It looks good.
Thank you for answers and code adjustment.
Thanks,
Serguei
On 1/31/14 1:48 AM, Dmitry Samersoff wrote:
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
