Skip to content

Commit

Permalink
Merge pull request #43548 from NipunaRanasinghe/debugger-bp-improvements
Browse files Browse the repository at this point in the history 
[Debugger] Optimize dynamic breakpoints processing flow in the debug server
  • Loading branch information
@NipunaRanasinghe
NipunaRanasinghe authored Nov 11, 2024
2 parents 5c9cc32 + 06e7fa7 commit 609ceb1
Show file tree
Hide file tree
Showing 8 changed files with 161 additions and 90 deletions.
93 changes: 54 additions & 39 deletions .../debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,15 @@
import com.sun.jdi.ThreadReference;
import com.sun.jdi.event.BreakpointEvent;
import com.sun.jdi.request.BreakpointRequest;
import com.sun.jdi.request.EventRequest;
import com.sun.jdi.request.StepRequest;
import org.ballerinalang.debugadapter.breakpoint.BalBreakpoint;
import org.ballerinalang.debugadapter.breakpoint.LogMessage;
import org.ballerinalang.debugadapter.breakpoint.TemplateLogMessage;
import org.ballerinalang.debugadapter.config.ClientConfigHolder;
import org.ballerinalang.debugadapter.config.ClientLaunchConfigHolder;
import org.ballerinalang.debugadapter.evaluation.BExpressionValue;
import org.ballerinalang.debugadapter.evaluation.DebugExpressionEvaluator;
import org.ballerinalang.debugadapter.evaluation.EvaluationException;
import org.ballerinalang.debugadapter.evaluation.EvaluationExceptionKind;
import org.ballerinalang.debugadapter.jdi.JDIUtils;
import org.ballerinalang.debugadapter.jdi.JdiProxyException;
import org.ballerinalang.debugadapter.jdi.StackFrameProxyImpl;
import org.ballerinalang.debugadapter.jdi.ThreadReferenceProxyImpl;
Expand All @@ -48,6 +46,7 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ConcurrentHashMap;
Expand Down Expand Up @@ -107,9 +106,10 @@ void processBreakpointEvent(BreakpointEvent bpEvent) {
// need to internally step out if we are at the last line of a function, in order to ignore having debug hits
// on the last line.
if (requireStepOut(bpEvent)) {
activateDynamicBreakPoints((int) bpEvent.thread().uniqueID(), DynamicBreakpointMode.CALLER);
activateDynamicBreakPoints((int) bpEvent.thread().uniqueID(), DynamicBreakpointMode.CALLER, true);
context.setPrevInstruction(DebugInstruction.STEP_OVER);
context.getDebuggeeVM().resume();
} else if (context.getLastInstruction() != null && context.getLastInstruction() != DebugInstruction.CONTINUE) {
} else if (context.getPrevInstruction() != null && context.getPrevInstruction() != DebugInstruction.CONTINUE) {
jdiEventProcessor.notifyStopEvent(bpEvent);
} else if (fileBreakpoints == null || !fileBreakpoints.containsKey(lineNumber)) {
jdiEventProcessor.notifyStopEvent(bpEvent);
Expand Down Expand Up @@ -168,19 +168,13 @@ private void processAdvanceBreakpoints(BreakpointEvent event, BalBreakpoint brea
/**
* Responsible for clearing dynamic(temporary) breakpoints used for step over instruction and for restoring the
* original user breakpoints before proceeding with the other debug instructions.
*
* @param instruction debug instruction
*/
void restoreUserBreakpoints(DebugInstruction instruction) {
void restoreUserBreakpoints() {
if (context.getDebuggeeVM() == null) {
return;
}

context.getEventManager().deleteAllBreakpoints();
if (instruction == DebugInstruction.CONTINUE || instruction == DebugInstruction.STEP_OVER) {
context.getDebuggeeVM().allClasses().forEach(referenceType ->
activateUserBreakPoints(referenceType, false));
}
context.getDebuggeeVM().allClasses().forEach(classRef -> activateUserBreakPoints(classRef, false));
}

/**
Expand All @@ -191,13 +185,6 @@ void restoreUserBreakpoints(DebugInstruction instruction) {
*/
void activateUserBreakPoints(ReferenceType referenceType, boolean shouldNotify) {
try {
// avoids setting break points if the server is running in 'no-debug' mode.
ClientConfigHolder configHolder = context.getAdapter().getClientConfigHolder();
if (configHolder instanceof ClientLaunchConfigHolder
&& ((ClientLaunchConfigHolder) configHolder).isNoDebugMode()) {
return;
}

String qualifiedClassName = getQualifiedClassName(referenceType);
if (!userBreakpoints.containsKey(qualifiedClassName)) {
return;
Expand All @@ -206,6 +193,7 @@ void activateUserBreakPoints(ReferenceType referenceType, boolean shouldNotify)
for (BalBreakpoint breakpoint : breakpoints.values()) {
List<Location> locations = referenceType.locationsOfLine(breakpoint.getLine());
if (!locations.isEmpty()) {
// TODO: should we consider the last location instead?
Location loc = locations.get(0);
BreakpointRequest bpReq = context.getEventManager().createBreakpointRequest(loc);
bpReq.enable();
Expand All @@ -230,23 +218,32 @@ void activateUserBreakPoints(ReferenceType referenceType, boolean shouldNotify)
* Activates dynamic/temporary breakpoints (which will be used to process the STEP-OVER instruction) via Java Debug
* Interface(JDI).
*
* @param threadId ID of the active java thread in oder to configure dynamic breakpoint on the active stack trace
* @param mode dynamic breakpoint mode
* @param threadId ID of the active java thread in oder to configure dynamic breakpoint on the active stack
* trace.
* @param mode dynamic breakpoint mode.
* @param validate If true, validates whether the dynamic breakpoints are already applied before activating dynamic
* breakpoints. This is to optimize the performance by avoiding redundant breakpoint activations.
*/
void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode) {
ThreadReferenceProxyImpl threadReference = context.getAdapter().getAllThreads().get(threadId);
void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode, boolean validate) {
try {
ThreadReferenceProxyImpl threadReference = context.getAdapter().getAllThreads().get(threadId);
List<StackFrameProxyImpl> jStackFrames = threadReference.frames();
List<BallerinaStackFrame> validFrames = jdiEventProcessor.filterValidBallerinaFrames(jStackFrames);

if (mode == DynamicBreakpointMode.CURRENT && !validFrames.isEmpty()) {
configureBreakpointsForMethod(validFrames.get(0));
Location currentLocation = validFrames.get(0).getJStackFrame().location();
Optional<Location> prevLocation = context.getPrevLocation();
if (!validate || prevLocation.isEmpty() || !isWithinSameSource(currentLocation, prevLocation.get())) {
doActivateDynamicBreakPoints(currentLocation);
}
context.setPrevLocation(currentLocation);
}

// If the current function is invoked within another ballerina function, we need to explicitly set another
// temporary breakpoint on the location of its invocation. This is supposed to handle the situations where
// the user wants to step over on an exit point of the current function.
if (mode == DynamicBreakpointMode.CALLER && validFrames.size() > 1) {
configureBreakpointsForMethod(validFrames.get(1));
doActivateDynamicBreakPoints(validFrames.get(1).getJStackFrame().location());
}
} catch (JdiProxyException e) {
LOGGER.error(e.getMessage());
Expand All @@ -257,16 +254,35 @@ void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode) {
}
}

private void doActivateDynamicBreakPoints(Location location) {
context.getEventManager().deleteAllBreakpoints();
configureBreakpointsForMethod(location);
context.setPrevLocation(location);
}

/**
* Checks whether the given two locations are within the same source file.
*
* @param currentLocation current location
* @param prevLocation previous location
* @return true if the given two locations are within the same source file, false otherwise
*/
private boolean isWithinSameSource(Location currentLocation, Location prevLocation) {
try {
return Objects.equals(currentLocation.sourcePath(), prevLocation.sourcePath());
} catch (AbsentInformationException e) {
return false;
}
}

/**
* Configures temporary(dynamic) breakpoints for all the lines within the method, which encloses the given stack
* frame location. This strategy is used when processing STEP_OVER requests.
*
* @param balStackFrame stack frame which contains the method information
* @param currentLocation current stack frame location
*/
private void configureBreakpointsForMethod(BallerinaStackFrame balStackFrame) {
private void configureBreakpointsForMethod(Location currentLocation) {
try {
Location currentLocation = balStackFrame.getJStackFrame().location();
ReferenceType referenceType = currentLocation.declaringType();
List<Location> allLocations = currentLocation.method().allLineLocations();
Optional<Location> firstLocation = allLocations.stream()
.filter(location -> location.lineNumber() > 0)
Expand All @@ -278,7 +294,7 @@ private void configureBreakpointsForMethod(BallerinaStackFrame balStackFrame) {

int nextStepPoint = firstLocation.get().lineNumber();
do {
List<Location> locations = referenceType.locationsOfLine(nextStepPoint);
List<Location> locations = currentLocation.method().locationsOfLine(nextStepPoint);
if (!locations.isEmpty() && (locations.get(0).lineNumber() > firstLocation.get().lineNumber())) {
// Checks whether there are any user breakpoint configured for the same location, before adding the
// dynamic breakpoint.
Expand All @@ -291,7 +307,7 @@ private void configureBreakpointsForMethod(BallerinaStackFrame balStackFrame) {
}
nextStepPoint++;
} while (nextStepPoint <= lastLocation.get().lineNumber());
} catch (AbsentInformationException | JdiProxyException e) {
} catch (AbsentInformationException e) {
LOGGER.error(e.getMessage());
}
}
Expand Down Expand Up @@ -362,8 +378,7 @@ private BExpressionValue evaluateExpressionSafely(String expression, ThreadRefer
// the new event. If this new EventSet is in 'SUSPEND_ALL' mode, then a deadlock will occur because no one
// will resume the EventSet. Therefore to avoid this, we are disabling possible event requests before doing
// the condition evaluation.
context.getEventManager().classPrepareRequests().forEach(EventRequest::disable);
context.getEventManager().breakpointRequests().forEach(BreakpointRequest::disable);
JDIUtils.disableJDIRequests(context);

ThreadReferenceProxyImpl thread = context.getAdapter().getAllThreads().get((int) threadReference.uniqueID());
List<BallerinaStackFrame> validFrames = jdiEventProcessor.filterValidBallerinaFrames(thread.frames());
Expand All @@ -377,16 +392,16 @@ private BExpressionValue evaluateExpressionSafely(String expression, ThreadRefer
evaluator.setExpression(expression);
BExpressionValue evaluationResult = evaluator.evaluate();

// As we are disabling all the breakpoint requests before evaluating the user's conditional
// As we disabled all the breakpoint requests before evaluating the user's conditional
// expression, need to re-enable all the breakpoints before continuing the remote VM execution.
restoreUserBreakpoints(context.getLastInstruction());
JDIUtils.enableJDIRequests(context);
return evaluationResult;
}

private boolean requireStepOut(BreakpointEvent event) {
try {
if (context.getLastInstruction() != DebugInstruction.STEP_OVER
&& context.getLastInstruction() != DebugInstruction.STEP_OUT) {
if (context.getPrevInstruction() != DebugInstruction.STEP_OVER
&& context.getPrevInstruction() != DebugInstruction.STEP_OUT) {
return false;
}
Location currentLocation = event.location();
Expand Down Expand Up @@ -415,7 +430,7 @@ private void notifyBreakPointChangesToClient(BalBreakpoint balBreakpoint) {
/**
* Dynamic Breakpoint Options.
*/
enum DynamicBreakpointMode {
public enum DynamicBreakpointMode {
/**
* Configures dynamic breakpoints only for the current method (active stack frame).
*/
Expand Down
23 changes: 17 additions & 6 deletions ...les/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/ExecutionContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package org.ballerinalang.debugadapter;

import com.sun.jdi.Location;
import com.sun.jdi.request.EventRequestManager;
import io.ballerina.projects.Project;
import org.ballerinalang.debugadapter.jdi.VirtualMachineProxyImpl;
Expand All @@ -39,14 +40,16 @@ public class ExecutionContext {
private String sourceProjectRoot;
private final DebugProjectCache projectCache;
private Process launchedProcess;
private DebugInstruction lastInstruction;
private boolean terminateRequestReceived;
private boolean supportsRunInTerminalRequest;
private DebugInstruction prevInstruction;
private Location prevLocation;

ExecutionContext(JBallerinaDebugServer adapter) {
this.adapter = adapter;
this.projectCache = new DebugProjectCache();
this.lastInstruction = DebugInstruction.CONTINUE;
this.prevInstruction = DebugInstruction.CONTINUE;
this.prevLocation = null;
}

public Optional<Process> getLaunchedProcess() {
Expand Down Expand Up @@ -102,12 +105,20 @@ public BufferedReader getErrorStream() {
return new BufferedReader(new InputStreamReader(launchedProcess.getErrorStream(), StandardCharsets.UTF_8));
}

public DebugInstruction getLastInstruction() {
return lastInstruction;
public DebugInstruction getPrevInstruction() {
return prevInstruction;
}

public void setLastInstruction(DebugInstruction lastInstruction) {
this.lastInstruction = lastInstruction;
public void setPrevInstruction(DebugInstruction prevInstruction) {
this.prevInstruction = prevInstruction;
}

public Optional<Location> getPrevLocation() {
return Optional.ofNullable(prevLocation);
}

public void setPrevLocation(Location prevLocation) {
this.prevLocation = prevLocation;
}

public DebugMode getDebugMode() {
Expand Down
40 changes: 27 additions & 13 deletions ...ebug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JBallerinaDebugServer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import io.ballerina.identifier.Utils;
import io.ballerina.projects.Project;
import io.ballerina.projects.directory.SingleFileProject;
import org.ballerinalang.debugadapter.BreakpointProcessor.DynamicBreakpointMode;
import org.ballerinalang.debugadapter.breakpoint.BalBreakpoint;
import org.ballerinalang.debugadapter.completion.CompletionGenerator;
import org.ballerinalang.debugadapter.completion.context.CompletionContext;
Expand Down Expand Up @@ -215,6 +216,11 @@ public CompletableFuture<Capabilities> initialize(InitializeRequestArguments arg
@Override
public CompletableFuture<SetBreakpointsResponse> setBreakpoints(SetBreakpointsArguments args) {
return CompletableFuture.supplyAsync(() -> {
SetBreakpointsResponse bpResponse = new SetBreakpointsResponse();
if (isNoDebugMode()) {
return bpResponse;
}

BalBreakpoint[] balBreakpoints = Arrays.stream(args.getBreakpoints())
.map((SourceBreakpoint sourceBreakpoint) -> toBreakpoint(sourceBreakpoint, args.getSource()))
.toArray(BalBreakpoint[]::new);
Expand All @@ -224,7 +230,6 @@ public CompletableFuture<SetBreakpointsResponse> setBreakpoints(SetBreakpointsAr
breakpointsMap.put(bp.getLine(), bp);
}

SetBreakpointsResponse bpResponse = new SetBreakpointsResponse();
String sourcePathUri = args.getSource().getPath();
Optional<String> qualifiedClassName = getQualifiedClassName(context, sourcePathUri);
if (qualifiedClassName.isEmpty()) {
Expand Down Expand Up @@ -425,7 +430,7 @@ public CompletableFuture<SourceResponse> source(SourceArguments args) {

@Override
public CompletableFuture<ContinueResponse> continue_(ContinueArguments args) {
prepareFor(DebugInstruction.CONTINUE);
prepareFor(DebugInstruction.CONTINUE, args.getThreadId());
context.getDebuggeeVM().resume();
ContinueResponse continueResponse = new ContinueResponse();
continueResponse.setAllThreadsContinued(true);
Expand All @@ -434,29 +439,25 @@ public CompletableFuture<ContinueResponse> continue_(ContinueArguments args) {

@Override
public CompletableFuture<Void> next(NextArguments args) {
prepareFor(DebugInstruction.STEP_OVER);
prepareFor(DebugInstruction.STEP_OVER, args.getThreadId());
eventProcessor.sendStepRequest(args.getThreadId(), StepRequest.STEP_OVER);
return CompletableFuture.completedFuture(null);
}

@Override
public CompletableFuture<Void> stepIn(StepInArguments args) {
prepareFor(DebugInstruction.STEP_IN);
prepareFor(DebugInstruction.STEP_IN, args.getThreadId());
eventProcessor.sendStepRequest(args.getThreadId(), StepRequest.STEP_INTO);
return CompletableFuture.completedFuture(null);
}

@Override
public CompletableFuture<Void> stepOut(StepOutArguments args) {
stepOut(args.getThreadId());
prepareFor(DebugInstruction.STEP_OUT, args.getThreadId());
eventProcessor.sendStepRequest(args.getThreadId(), StepRequest.STEP_OUT);
return CompletableFuture.completedFuture(null);
}

void stepOut(int threadId) {
prepareFor(DebugInstruction.STEP_OUT);
eventProcessor.sendStepRequest(threadId, StepRequest.STEP_OUT);
}

@Override
public CompletableFuture<Void> setExceptionBreakpoints(SetExceptionBreakpointsArguments args) {
return CompletableFuture.completedFuture(null);
Expand Down Expand Up @@ -1119,10 +1120,23 @@ private void attachToRemoteVM(String hostName, int portName) throws IOException,
/**
* Clears previous state information and prepares for the given debug instruction type execution.
*/
private void prepareFor(DebugInstruction instruction) {
private void prepareFor(DebugInstruction instruction, int threadId) {
clearState();
eventProcessor.getBreakpointProcessor().restoreUserBreakpoints(instruction);
context.setLastInstruction(instruction);
BreakpointProcessor bpProcessor = eventProcessor.getBreakpointProcessor();
DebugInstruction prevInstruction = context.getPrevInstruction();
if (prevInstruction == DebugInstruction.STEP_OVER && instruction == DebugInstruction.CONTINUE) {
bpProcessor.restoreUserBreakpoints();
} else if (prevInstruction == DebugInstruction.CONTINUE && instruction == DebugInstruction.STEP_OVER) {
bpProcessor.activateDynamicBreakPoints(threadId, DynamicBreakpointMode.CURRENT, false);
} else if (instruction == DebugInstruction.STEP_OVER) {
bpProcessor.activateDynamicBreakPoints(threadId, DynamicBreakpointMode.CURRENT, true);
}
context.setPrevInstruction(instruction);
}

private boolean isNoDebugMode() {
ClientConfigHolder confHolder = context.getAdapter().getClientConfigHolder();
return confHolder instanceof ClientLaunchConfigHolder launchConfigHolder && launchConfigHolder.isNoDebugMode();
}

/**
Expand Down
Loading

0 comments on commit 609ceb1

Please sign in to comment.