Promise breakpoint #58

ctrlpz · 2016-11-22T14:24:26Z

Implementation of promise breakpoints for resolver and resolution side.

smarr

Overall looks good. Most comments are polish to keep consistent with existing style and avoid code duplication.

smarr · 2016-11-22T14:47:55Z

npm-debug.log

@@ -0,0 +1,22 @@
+0 info it worked if it ends with ok


please do not commit such logs

smarr · 2016-11-22T14:50:28Z

src/som/interpreter/actors/EventualMessage.java


  protected EventualMessage(final EventualMessage causalMessage, final Object[] args,
-      final SResolver resolver, final RootCallTarget onReceive, final boolean triggerRcvrBreakpoint) {
+      final SResolver resolver, final RootCallTarget onReceive, final boolean triggerRcvrBreakpoint, final boolean triggerPromiseResolveBreakpoint, final boolean triggerPromiseResolveReceiverBreakpoint) {


please break the line after about 80 characters

smarr · 2016-11-22T14:50:46Z

src/som/interpreter/actors/EventualMessage.java

-        final RootCallTarget onReceive, final boolean triggerRcvrBreakpoint) {
-      super(causalMessage, arguments, resolver, onReceive, triggerRcvrBreakpoint);
+        final RootCallTarget onReceive, final boolean triggerRcvrBreakpoint, final boolean triggerPromiseResolveSenderBreakpoint, final boolean triggerPromiseResolveReceiverBreakpoint) {
+      super(causalMessage, arguments, resolver, onReceive, triggerRcvrBreakpoint, triggerPromiseResolveSenderBreakpoint, triggerPromiseResolveReceiverBreakpoint);


please break the line after about 80 characters

smarr · 2016-11-22T14:51:49Z

src/som/interpreter/actors/EventualMessage.java

@@ -22,15 +22,19 @@
  protected final RootCallTarget onReceive;
  protected final EventualMessage causalMessage;

-  protected final boolean triggerRcvrBreakpoint;
+  protected boolean triggerRcvrBreakpoint;


would be good to add a comment to the field explaining why it is not final like the other ones.

smarr · 2016-11-22T14:52:18Z

src/som/interpreter/actors/EventualMessage.java

@@ -22,15 +22,19 @@
  protected final RootCallTarget onReceive;
  protected final EventualMessage causalMessage;

-  protected final boolean triggerRcvrBreakpoint;
+  protected boolean triggerRcvrBreakpoint;
+  protected final boolean triggerPromiseResolveSenderBreakpoint;


didn't we have new terminology after the discussion with Elisa? would be good to change the code accordingly.

smarr · 2016-11-22T15:02:49Z

src/tools/debugger/nodes/BreakpointPromiseRcvNode.java

@@ -0,0 +1,39 @@
+package tools.debugger.nodes;


Please refactor these classes to avoid code duplication. The only thing that seems to be different is how the BreakpointEnabling object is retrieved. This can be factored out.

to refactor the BreakpointNode classes I think there is a problem because of the types of breakpoints. Indeed the BreakpointEnabling object is the one that makes the difference, but to be able to use only one BreakpointNode class I think that we will have to use a superclass and this will bring some changes because the most part of implementation related to breakpoints is specified by each type of breakpoint.

smarr · 2016-11-22T15:03:09Z

src/tools/debugger/nodes/DisabledBreakpointNode.java

+ * Disabled breakpoint node that returns false all the time.
+ *
+ * (use mainly when the Truffle debugger is not enabled)
+ *


remove empty line

smarr · 2016-11-22T15:04:45Z

tools/kompos/src/controller.ts

@@ -74,6 +74,8 @@ export class Controller {
          case "AsyncMessageReceiveBreakpoint":
            this.view.updateAsyncMethodRcvBreakpoint(<AsyncMethodRcvBreakpoint> bp);
            break;
+          case "PromiseSenderBreakpoint":
+          case "PromiseReceiveBreakpoint": /** TODO */


Is this correct? looks like there is code missing

smarr · 2016-11-22T15:07:08Z

tools/kompos/src/messages.ts

-    line:      line,
-    sourceUri: sourceUri,
-    enabled:   false};
+export type Breakpoint = LineBreakpoint | MessageBreakpoint |


It looks like this is a broken merge/rebase.
Please see the breakpoints.ts file next to messages.ts. This here seem to duplicate the code that moved there.

smarr · 2016-11-22T15:09:19Z

tools/kompos/src/messages.ts

+    source, sourceId, clickedSpan);
+}
+
+export function createMsgBreakpoint(source: Source,


If the only difference between the various breakpoints is here the type. Please factor out this code to create the object that is passed to the constructor into a helper method to avoid code duplication.

smarr

Some naming, cosmetics, and the breakpoint class

smarr · 2016-11-23T14:29:26Z

src/som/interpreter/actors/EventualMessage.java

+    * It is not final because its value can be updated in order that other breakpoints
+    * can reuse the stepping strategy implemented for message receiver breakpoints.
+    */
+  protected boolean triggerRcvrBreakpoint;


triggerRcvrBreakpoint should be named triggerMessageReceiverBreakpoint to be unambiguous within the new set of breakpoints

smarr · 2016-11-23T14:29:57Z

src/som/interpreter/actors/EventualMessage.java

@@ -131,8 +144,8 @@ protected static Actor determineTargetAndWrapArguments(final Object[] arguments,
    protected final Actor originalSender; // initial owner of the arguments

    public PromiseMessage(final EventualMessage causalMessage, final Object[] arguments, final Actor originalSender,
-        final SResolver resolver, final RootCallTarget onReceive, final boolean triggerRcvrBreakpoint) {
-      super(causalMessage, arguments, resolver, onReceive, triggerRcvrBreakpoint);
+        final SResolver resolver, final RootCallTarget onReceive, final boolean triggerRcvrBreakpoint, final boolean triggerPromiseResolverBreakpoint, final boolean triggerPromiseResolutionBreakpoint) {


please keep lines within 80 chars

smarr · 2016-11-23T14:30:55Z

src/som/interpreter/actors/EventualSendNode.java

@@ -114,7 +112,9 @@ public static boolean isParentResultUsed(final Node current, final Node child) {

    protected final SourceSection source;

-    protected final AbstractBreakpointNode breakpoint;
+    protected final AbstractBreakpointNode messageReceivedBreakpoint;


messageReceivedBreakpoint -> messageReceiverBreakpoint

smarr · 2016-11-23T14:32:24Z

src/som/interpreter/actors/ReceivedRootNode.java

    public NullResolver(final SourceSection source) {
      super(false, source);
    }

    @Override
-    public Object executeEvaluated(final VirtualFrame frame, final SResolver receiver, final Object argument) {
+    public Object executeEvaluated(final VirtualFrame frame, final SResolver receiver, final Object argument, final boolean isBreakpointOnResolutionAtSender, final boolean isBreakpointOnResolutionAtRcvr) {


please keep it to 80chars

smarr · 2016-11-23T14:33:21Z

src/som/interpreter/actors/ReceivedRootNode.java

+      if (isBreakpointOnResolutionAtSender) {
+        haltNode.executeEvaluated(frame, argument);
+      }
+      if (isBreakpointOnResolutionAtRcvr) {


yeah, please avoid committing your todo items as empty if statements

smarr · 2016-11-23T14:38:51Z

src/tools/debugger/nodes/BreakpointNode.java

+    protected final BreakpointEnabling<?> breakpoint;
+
+    protected BreakpointNode(final SourceSection sourceSection, final MessageBreakpointType type) {
+      switch (type) {


Please move this out of the constructor to the corresponding code and please don't use a switch.

smarr · 2016-11-23T14:41:04Z

src/tools/debugger/nodes/BreakpointNode.java

+
+    /** Only to be used by the DisabledBreakpointNode. */
+    protected BreakpointNode() {
+      this.breakpoint = null;


This is not used anymore since DisabledBreakpoint inherits directly from AbstractBreakpointNode, right?
If so, please remove.

smarr · 2016-11-23T14:42:19Z

src/tools/debugger/session/Breakpoints.java

      receiverBreakpoints.put(coord, existingBP);
    } else {
      existingBP.setEnabled(bId.isEnabled());
    }
  }

+  public synchronized void addOrUpdate(final PromiseResolverBreakpoint bId) {


this code looks repetitive, looks like one could factor out a helper method here.

I refactored the addOrUpdate for MessageSenderBreakpoint and AsyncMessageReceiverBreakpoint...But for the case of MessageReceiverBreakpoint and the promises breakpoints I have been trying to do something like this:

public synchronized void addOrUpdate(final PromiseResolutionBreakpoint bId) { saveBreakpointsEnabling(bId, promiseResolutionBreakpoints); } private void saveBreakpointsEnabling(final SectionBreakpoint bId, final Map<FullSourceCoordinate, BreakpointEnabling<? extends SectionBreakpoint>> map) { FullSourceCoordinate coord = bId.getCoordinate(); BreakpointEnabling<? extends SectionBreakpoint> existingBP = map.get(coord); if (existingBP == null) { existingBP = new BreakpointEnabling<>(bId); map.put(coord, existingBP); } else { existingBP.setEnabled(bId.isEnabled()); } }

the problem is that the argument types are not applicable for '? extends SectionBreakpoint', I'm not sure if there is a way of refactoring these three methods. The only difference between them is the map (receiverBreakpoints, promiseResolverBreakpoints, promiseResolutionBreakpoints)

At this point you don't really need the info about extends SectionBreakpoint or do you?
Guess it would work if you drop that part?

it keeps saying it is not applicable for the arguments :(

private void saveBreakpointsEnabling(final SectionBreakpoint bId, final Map<FullSourceCoordinate, BreakpointEnabling<?>> map) { FullSourceCoordinate coord = bId.getCoordinate(); BreakpointEnabling<?> existingBP = map.get(coord); if (existingBP == null) { existingBP = new BreakpointEnabling<>(bId); map.put(coord, existingBP); } else { existingBP.setEnabled(bId.isEnabled()); } }

smarr · 2016-11-23T14:43:17Z

src/tools/debugger/nodes/BreakpointNode.java

+ * It has two possible states, enable or disable.
+ */
+public abstract class BreakpointNode extends AbstractBreakpointNode {
+    protected final BreakpointEnabling<?> breakpoint;


Please make this a proper generic type. Replace ? with T. And add T as a generic argument to BreakpointNode

the generic argument T should extend of BreakpointInfo too as BreakpointEnabling?

T extends BreakpointInfo seems useful, yes

smarr · 2016-11-23T14:44:56Z

tools/kompos/src/messages.ts

@@ -98,7 +98,7 @@ export interface FarRefData {
 export type BreakpointData = LineBreakpointData | SectionBreakpointData;

 export type SectionBreakpointType = "MessageSenderBreakpoint" |
-  "MessageReceiveBreakpoint" | "AsyncMessageReceiveBreakpoint";
+  "MessageReceiverBreakpoint" | "AsyncMessageReceiverBreakpoint" | "PromiseResolverBreakpoint" | "PromiseResolutionBreakpoint" ;


please keep within 80 chars per line

smarr · 2016-11-24T15:25:07Z

src/tools/debugger/session/Breakpoints.java

      bp = Breakpoint.newBuilder(bId.getCoordinate().uri).
          lineIs(bId.getCoordinate().startLine).
          columnIs(bId.getCoordinate().startColumn).
          sectionLength(bId.getCoordinate().charLength).
          build();
-      bp.setCondition(BreakWhenActivatedByAsyncMessage.INSTANCE);


@ctrlpz this is a problem, this line is essential :( and we really need tests for these things

sorry, I did not notice I removed it...yes we need the tests

- add options to frontend for promise breakpoint on sender and receiver side - change button names in popover - add breakpoint classes - move classes related to breakpoint node from EventualSendNode to an independent package - add SuspendExecutionNode for halting the execution before the promise is resolved (todo: needs to be replaced by stepping strategy) - add helper method to create SectionBreakpointData object to avoid code duplication between the different breakpoints - remove CoverallsTruffle from Eclipse classpath (project is already part of SOMns-deps

Signed-off-by: Stefan Marr <git@stefan-marr.de>

ctrlpz force-pushed the promise-breakpoint branch from 72889ec to fcb6c45 Compare November 22, 2016 14:45

smarr requested changes Nov 22, 2016

View reviewed changes

smarr requested changes Nov 23, 2016

View reviewed changes

ctrlpz force-pushed the promise-breakpoint branch from f7478e9 to 4bd7fb0 Compare November 24, 2016 11:03

smarr force-pushed the promise-breakpoint branch from 4bd7fb0 to af9cf57 Compare November 24, 2016 15:00

smarr reviewed Nov 24, 2016

View reviewed changes

ctrlpz and others added 2 commits November 24, 2016 16:54

Update dynamic metrics test data

f99f5a8

Signed-off-by: Stefan Marr <git@stefan-marr.de>

smarr force-pushed the promise-breakpoint branch from 3d5f295 to f99f5a8 Compare November 24, 2016 15:55

smarr approved these changes Nov 24, 2016

View reviewed changes

smarr mentioned this pull request Nov 24, 2016

Add tests for all not yet tested breakpoints to kompos tests #62

Closed

smarr force-pushed the promise-breakpoint branch from 7410cf4 to f99f5a8 Compare November 24, 2016 16:41

smarr merged commit b46f6bb into smarr:master Nov 24, 2016

smarr deleted the promise-breakpoint branch November 24, 2016 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promise breakpoint #58

Promise breakpoint #58

ctrlpz commented Nov 22, 2016

smarr left a comment

smarr Nov 22, 2016

smarr Nov 22, 2016

smarr Nov 22, 2016

smarr Nov 22, 2016

smarr Nov 22, 2016

smarr Nov 22, 2016

ctrlpz Nov 23, 2016

smarr Nov 22, 2016

smarr Nov 22, 2016

smarr Nov 22, 2016

smarr Nov 22, 2016

smarr left a comment

smarr Nov 23, 2016

smarr Nov 23, 2016

smarr Nov 23, 2016

smarr Nov 23, 2016

smarr Nov 23, 2016

smarr Nov 23, 2016

smarr Nov 23, 2016

smarr Nov 23, 2016

ctrlpz Nov 24, 2016 •

edited by smarr

smarr Nov 24, 2016

ctrlpz Nov 24, 2016 •

edited

smarr Nov 23, 2016

ctrlpz Nov 23, 2016

smarr Nov 23, 2016

smarr Nov 23, 2016

smarr Nov 24, 2016

ctrlpz Nov 24, 2016

Promise breakpoint #58

Promise breakpoint #58

Conversation

ctrlpz commented Nov 22, 2016

smarr left a 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

smarr left a 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

ctrlpz Nov 24, 2016 • edited by smarr

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctrlpz Nov 24, 2016 • edited

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

ctrlpz Nov 24, 2016 •

edited by smarr

ctrlpz Nov 24, 2016 •

edited