PR2 of conditional BP #154

andxu · 2018-03-12T03:27:33Z

Fix the issue when the condition is updated
Add a cache to the compiled expression for conditional breakpoint.

yaohaizh · 2018-03-12T03:44:16Z

...t.java.debug.core/src/main/java/com/microsoft/java/debug/core/adapter/BreakpointManager.java

@@ -31,6 +31,9 @@
    private List<IBreakpoint> breakpoints;
    private HashMap<String, HashMap<String, IBreakpoint>> sourceToBreakpoints;
    private AtomicInteger nextBreakpointId = new AtomicInteger(1);
+    // BreakpointManager is the owner class of the breakpoint to compiled expression map, it will remove
+    // the breakpoint from this map if the breakpoint is removed or changing its condition


if the breakpoint is removed or changing its condition ==> if the breakpoint is removed or its condition is changed

…am is stopped at the exception 2. update a comment

yaohaizh · 2018-03-12T04:01:56Z

...rc/main/java/com/microsoft/java/debug/core/adapter/handler/SetBreakpointsRequestHandler.java

+                    }
+
+                    if (!StringUtils.equals(toAdds[i].getCondition(), added[i].getCondition())) {
+                        manager.updateConditionCompiledExpression(added[i], toAdds[i].getCondition());


It is strange put the hitCount update in here while keep the condition update in other places. The logic is mixed.

Because updating condition will need to clear the expression cache.

setHitCount contains the logic to update hit count in jvm, it is not a pure set function.

yaohaizh · 2018-03-12T04:13:18Z

...lugin/src/main/java/com/microsoft/java/debug/plugin/internal/eval/JdtEvaluationProvider.java

+            }
+
+            if (project == null) {
+                new IllegalStateException(String.format("Project %s cannot be found.", projectName));


Why new a unused exception?

will use exception for the failure of initialization to debugTarget

yaohaizh · 2018-03-12T04:15:06Z

...lugin/src/main/java/com/microsoft/java/debug/plugin/internal/eval/JdtEvaluationProvider.java

+            }
+        }
+
+        if (debugTarget == null) {


The outside is already have this logic

debugTarget == null

Why do we have two if blocks with the same condition to check whether debugTarget is null?

Old issue in previous code, will fix it.

yaohaizh · 2018-03-12T05:02:03Z

...lugin/src/main/java/com/microsoft/java/debug/plugin/internal/eval/JdtEvaluationProvider.java

@@ -178,6 +188,37 @@ public void clearState(ThreadReference thread) {
        }
    }

+    private boolean ensureDebugTarget(VirtualMachine vm) {
+        String projectName = (String) options.get(Constants.PROJECTNAME);


If debugTarget is not null, this line is not necessary ?

2. fix the issue of a dirty evaluation environment cause the next evaluation failure.

yaohaizh · 2018-03-12T05:33:59Z

...t.java.debug.core/src/main/java/com/microsoft/java/debug/core/adapter/BreakpointManager.java

+     *
+     * @return the compiled expression map
+     */
+    public Map<IBreakpoint, Object> getBreakpointExpressionMap() {


If the cache is only used by the evaluation Provider, should it be better put insider the evaluation provider?

the keys are managed by breakpoint manager, and the values are managed by evaluation evaluation provider, I don't think its better for breakpoint manager to reference JdtEvaluationProvider

JDT are storing expression in JavaLineBreakpoint@fCompiledExpressions

/** * The map of cached compiled expressions (ICompiledExpression) for this * breakpoint, keyed by thread. This value must be cleared every time the * breakpoint is added to a target. */ private Map<IJavaThread, ICompiledExpression> fCompiledExpressions = new HashMap<>();

yaohaizh · 2018-03-12T05:39:33Z

...lugin/src/main/java/com/microsoft/java/debug/plugin/internal/eval/JdtEvaluationProvider.java

+            ASTEvaluationEngine engine = new ASTEvaluationEngine(project, debugTarget);
+            ICompiledExpression ie = (ICompiledExpression) breakpointExpressionMap
+                    .computeIfAbsent(breakpoint, bp -> engine.getCompiledExpression(bp.getCondition(), stackframe));
+


Return failureFuture and internalEvaluate result are two different instance. The client cannot hoop up with both future.

done with a CompletableFuture parameter in internalEvaluate

akaroml · 2018-03-12T05:40:19Z

...t.java.debug.core/src/main/java/com/microsoft/java/debug/core/adapter/BreakpointManager.java

@@ -31,6 +31,9 @@
    private List<IBreakpoint> breakpoints;
    private HashMap<String, HashMap<String, IBreakpoint>> sourceToBreakpoints;
    private AtomicInteger nextBreakpointId = new AtomicInteger(1);
+    // BreakpointManager is the owner class of the breakpoint to compiled expression map, it will remove
+    // the breakpoint from this map if the breakpoint is removed or its condition is changed
+    private Map<IBreakpoint, Object> breakpointExpressionMap = new HashMap<>();


Why is the breakpoint manager maintaining the cache map but not the evaluation engine?

The cache should be kept by evaluation engine. The BreakpointManger notifies the engine to invalidate cache entries.

Because evaluation engine has no information about when the breakpoint is removed or updated.

akaroml · 2018-03-12T05:45:22Z

...lugin/src/main/java/com/microsoft/java/debug/plugin/internal/eval/JdtEvaluationProvider.java

+            }
+        }
+
+        if (debugTarget == null) {


Why do we have two if blocks with the same condition to check whether debugTarget is null?

akaroml · 2018-03-12T05:49:37Z

...lugin/src/main/java/com/microsoft/java/debug/plugin/internal/eval/JdtEvaluationProvider.java

@@ -178,6 +188,37 @@ public void clearState(ThreadReference thread) {
        }
    }

+    private boolean ensureDebugTarget(VirtualMachine vm) {


The return value of ensureDebugTarget is never referenced anywhere. In the implementation, you use both exception and return value to indicate success, which is not recommended.

'ensure' means make sure something is actually there. When the function is not able to ensure that, an exception should be thrown. So please only use exceptions and the return value should be void.

akaroml · 2018-03-12T05:50:13Z

...lugin/src/main/java/com/microsoft/java/debug/plugin/internal/eval/JdtEvaluationProvider.java

-            if (launch == null) {
-                launch = createILaunchMock(project);
-            }
+    public CompletableFuture<Value> evaluateForBreakpoint(IBreakpoint breakpoint, ThreadReference thread, Map<IBreakpoint, Object> breakpointExpressionMap) {


What's the difference of evaluating and evaluating for breakpoints?

Because conditional breakpoint will be executed multiple time inside a loop(eval and cont), and the condition and location don't change, we need to optimize the conditional breakpoint, consider the following case, the i == 1000 will be evaluated 999 times before the breakpoint is to hit.

for (; i < 1000 + 1; i++) {
if (i == 1000) { # conditional BP of i == 1000
System.out.println( "1111" );
}
}

2. keep only one CompletableFuture in evaluate

Outdated

andxu added 5 commits March 9, 2018 15:05

Add basic impl of conditional BP

68554c0

remove an empty line.

6fbe232

change a better var name

e34190b

Merge remote-tracking branch 'origin' into andy_dev4

c1b771e

add cache for conditional breakpoint to avoid duplicate compilation

fe3493c

yaohaizh reviewed Mar 12, 2018

View reviewed changes

1. fix an issue when exception is thrown during evaluation, the progr…

60b5716

…am is stopped at the exception 2. update a comment

yaohaizh reviewed Mar 12, 2018

View reviewed changes

1. fix the issue findFirst will throw exception if none is found.

c555d11

2. fix the issue of a dirty evaluation environment cause the next evaluation failure.

yaohaizh reviewed Mar 12, 2018

View reviewed changes

andxu added 2 commits March 12, 2018 13:36

user filter to filter breakpoint event only

2e95990

rollback unfinished changes.

df8b3f7

yaohaizh reviewed Mar 12, 2018

View reviewed changes

akaroml previously requested changes Mar 12, 2018

View reviewed changes

1. make ensureDebugTarget void and use exception for the error handling

cae4adc

2. keep only one CompletableFuture in evaluate

yaohaizh approved these changes Mar 15, 2018

View reviewed changes

andxu merged commit 0b3da27 into master Mar 15, 2018

andxu deleted the andy_dev4 branch March 15, 2018 01:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR2 of conditional BP #154

PR2 of conditional BP #154

andxu commented Mar 12, 2018

yaohaizh Mar 12, 2018

yaohaizh Mar 12, 2018

andxu Mar 12, 2018

andxu Mar 12, 2018

yaohaizh Mar 12, 2018

andxu Mar 12, 2018

yaohaizh Mar 12, 2018

akaroml Mar 12, 2018

andxu Mar 12, 2018

yaohaizh Mar 12, 2018

andxu Mar 12, 2018

yaohaizh Mar 12, 2018

andxu Mar 12, 2018

andxu Mar 12, 2018

yaohaizh Mar 12, 2018

andxu Mar 12, 2018

akaroml Mar 12, 2018

akaroml Mar 12, 2018

andxu Mar 12, 2018

akaroml Mar 12, 2018

akaroml Mar 12, 2018

andxu Mar 12, 2018

akaroml Mar 12, 2018

andxu Mar 12, 2018 •

edited

Loading

PR2 of conditional BP #154

PR2 of conditional BP #154

Conversation

andxu commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andxu Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

andxu Mar 12, 2018 •

edited

Loading