Implement an initial version of instrumentation conditional breakpoints - #14
Alexey Merkulov (steelart) wants to merge 8 commits into
Conversation
e58873d to
4baefb5
Compare
|
|
||
| private static void addPoint(String propertyKey, String propertyValue) { | ||
| if (propertyKey.startsWith("instrumentation.condition.breakpoint")) { | ||
| String[] split = propertyValue.split(" "); |
There was a problem hiding this comment.
Classes, methods, and arguments might have spaces in their names (e.g., ReactorDebuggerWithAgentTest#test with debug agent()). We need something more sophisticated here.
It also affects stack trace capturing, but "capture" and "insertion" points are likely to be in some libraries or other core classes, luckily for us, without spaces. But breakpoints could be placed anywhere.
It might worth it, to develop a uniform way to pass these names to the agent.
There was a problem hiding this comment.
I propose to create a ticket and support this case a little bit later
There was a problem hiding this comment.
Please, don't forget to create and then resolve this.
| } | ||
|
|
||
| private static Map<Integer, InstrumentationBreakpointInfo> getLineNumbers(String methodName, Map<String, Map<Integer, InstrumentationBreakpointInfo>> methods) { | ||
| int index = methodName.indexOf("$lambda$"); |
There was a problem hiding this comment.
We absolutely need more comments here. It's unclear what's happening here.
There was a problem hiding this comment.
It is Kotlin lambda encoding. Also need to check similar cases for Java. Likely this place will be updated later. Also, it seems I'm not fully correct here. Like bp before func { inside() } will call twice now (instead of the old version). So, likely need to change the calling side so to extract the correct lambda name.
There was a problem hiding this comment.
I don't fully get it. But we still need more comments in the code.
| boolean isFound = false; | ||
| for (LocalVariableNode localVariableNode : method.localVariables) { | ||
| if (localVariableNode.name.equals(argumentName) && | ||
| method.instructions.indexOf(localVariableNode.start) <= instructionIndex && |
There was a problem hiding this comment.
I still don't like this approach with loading of the whole class into ClassNode tree. All this stuff could be done more efficiently using visitors. Is it your intentional choice or just a prototype?
There was a problem hiding this comment.
This code is executed only for classes with conditional breakpoints. I suspect that you never see a difference in real live between this implementation and the "optimized" one. I would preserve it for now it least. Also, I suspect, that the node representation might give us some other advantage.
There was a problem hiding this comment.
What other advantages? For me it looks like using the wrong tool for the task, it's not only about performance.
I have quite a strong opinion here, we might ask Egor for the third opinion.
…de information about conditional breakpoint check to engine
…stead of exception object
3feccd8 to
56bc7b2
Compare
| mv.visitJumpInsn(Opcodes.GOTO, afterIf); | ||
|
|
||
| mv.visitLabel(catchBlock); | ||
| mv.visitIntInsn(Opcodes.BIPUSH, instrumentationId); |
There was a problem hiding this comment.
Ok. Then please fix the comparison, instrumentationId == Short.MAX_VALUE isn't a problem. Anything greater is a problem. Otherwise it looks confusing.
|
|
||
| private static void addPoint(String propertyKey, String propertyValue) { | ||
| if (propertyKey.startsWith("instrumentation.condition.breakpoint")) { | ||
| String[] split = propertyValue.split(" "); |
There was a problem hiding this comment.
Please, don't forget to create and then resolve this.
| } | ||
|
|
||
| private static Map<Integer, InstrumentationBreakpointInfo> getLineNumbers(String methodName, Map<String, Map<Integer, InstrumentationBreakpointInfo>> methods) { | ||
| int index = methodName.indexOf("$lambda$"); |
There was a problem hiding this comment.
I don't fully get it. But we still need more comments in the code.
| Map<Integer, InstrumentationBreakpointInfo> lineNumbers = getLineNumbers(methodName, methods); | ||
| if (lineNumbers == null) { | ||
| lineNumbers = new LinkedHashMap<>(); | ||
| methods.put(methodName, lineNumbers); |
There was a problem hiding this comment.
String whereMethodName = methodName.substring(0, index);
return methods.get(whereMethodName);
But you get by substring there and put by full name here. What am I missing?
2 participants
No description provided.