[Issue 214] Exception signaling without helper methods #251

clementbera · 2018-05-22T16:53:54Z

This PR introduces the infrastructure to do exception signaling without relying on helper methods in the Newspeak code. Thereby, it addresses #214.

It introduces the ExceptionSignalingNode, which performs the message sends necessary to signal an exception.
In the interpreter, this allows us to have for instance code like this:

@Child ExceptionSignalingNode indexOutOfBounds;

// ...

indexOutOfBounds.signal(arr, idx);

Work with @daumayr.

This change does not yet fully optimize the exception signaling. For instance, it does not cache the exception class, which could avoid the message send for its lookup. However, this would need to be done very carefully, since the lookup is done via a normal message send, which could have side effects. Thus, it should only be optimized for well known and unproblematic cases.

[description updated by @smarr to be a summary of the change]

smarr · 2018-05-22T21:15:51Z

For attribution purposes, I'd add a line like Co-authored-by: name <[email protected]> to the commit message, to attribute Dominik. /cc @daumayr

See
https://help.github.com/articles/creating-a-commit-with-multiple-authors/

There's also a compilation error: https://travis-ci.org/smarr/SOMns/jobs/382258342#L689

smarr

Some comments and potential improvements.

smarr · 2018-05-22T21:25:07Z

core-lib/Kernel.ns

-
-  private signalArgumentError: message = (
-    ArgumentError signalWith: message.
-  )


Could we also remove https://github.com/smarr/SOMns/blob/release/core-lib/Files.ns#L664-L672 ?

smarr · 2018-05-22T21:26:08Z

src/som/interpreter/nodes/ExceptionSignalingNode.java

+  @Child protected ExpressionNode signalExceptionNode;
+  protected final SObject         module;
+
+  public ExceptionSignalingNode(final String exceptionClassName,


Hm, is that so common that it is worthwhile to have the standard parameter KernelObj.kernel?

There's a dozen senders, but we can remove it if you want to.

How about a named factory method?
I'd prefer that over a constructor, because it would be more explicit.

So, perhaps createForKernel(..)?

Moved to a factory as requested.

smarr · 2018-05-22T21:27:21Z

src/som/interpreter/nodes/ExceptionSignalingNode.java

+    allArgs[0] = exceptionClass;
+    for (int i = 0; i < args.length; i++) {
+      allArgs[i + 1] = args[i];
+    }


Would be nice to extract line 43-47 into a named method

smarr · 2018-05-22T21:34:36Z

src/som/interpreter/nodes/InstantiationNode.java

@@ -73,7 +98,8 @@ public SClass instantiateClass(final SObjectWithClass outerObj,
    public static SClass instantiate(final SObjectWithClass outerObj,
        final ClassFactory factory) {
      SClass classObj = new SClass(outerObj, instantiateMetaclassClass(factory, outerObj));
-      return singalExceptionsIfFaultFoundElseReturnClassObject(outerObj, factory, classObj);
+      return signalExceptionsIfFaultFoundElseReturnClassObject(outerObj, factory, classObj,
+          floatingNotAValueThrower, floatingCannotBeValuesThrower);


Hm, can we improve this?

Ideally, the nodes should come from the caller.
The @Specialization above could provide instance-specific nodes, and the other callers could pass them in from static fields, if really necessary.

smarr · 2018-05-22T21:35:36Z

src/som/interpreter/nodes/IsValueCheckNode.java

    }

+    @Child protected ExceptionSignalingNode thrower;


Fields are ideally defined before the constructors

smarr · 2018-05-22T21:37:17Z

src/som/primitives/ActivitySpawn.java

      return this;
    }

+    @Child protected ExceptionSignalingNode thrower;


Please before constructor

smarr · 2018-05-22T21:37:32Z

src/som/primitives/ActivitySpawn.java

      return this;
    }

+    @Child protected ExceptionSignalingNode thrower;


Please before constructor

smarr · 2018-05-22T21:38:56Z

src/som/primitives/BlockPrims.java

+    @Child ExceptionSignalingNode thrower;
+
+    @Override
+    public void signalException(final String message) {


Hm, can this be done as a default method to avoid code duplication?

We can't.
We are not able to use a common superclass.
If we move the code out, methods such as insert are not visible anymore.

smarr · 2018-05-22T21:39:54Z

src/som/primitives/ExceptionsPrims.java

@@ -103,4 +103,5 @@ public final Object doException(final SBlock body, final SBlock ensureHandler) {
      }
    }
  }
+


unnecessary change

Don't know how to remove that. CheckStyle added it for us I think.

smarr · 2018-05-22T21:42:38Z

src/som/vmobjects/SFileDescriptor.java

-    CompilerAsserts.neverPartOfCompilation();
-    KernelObj.signalException("signalArgumentError:", "File access mode invalid, was: "
-        + Objects.toString(mode) + " " + AccessModes.VALID_MODES);
+    this.accessMode = AccessModes.valueOf(mode.getString());


I don't get this change. Where did the exception go?

In the sender, so it can have the exceptionSignalNode.

smarr

@clementbera @daumayr I also just pushed a bunch of changes. They are mostly polish, stuff I didn't see from simple reviewing. Note, this is work in progress. Will continue later.

smarr · 2018-05-23T14:02:50Z

core-lib/Files.ns

@@ -645,6 +645,7 @@ DAMAGE.>> *)
    )
  ) : (
    public signalWith: aMessage = (
+      vmMirror printStackTrace: nil.


That's debugging code, we want to solve this not specific to an exception, but somewhat differently (there is no implementation of stack trace capturing at the moment)

smarr · 2018-05-28T11:08:16Z

src/som/primitives/SystemPrims.java

    } catch (NotAFileException e) {
      PathPrims.signalFileNotFoundException(e.getMessage(),
-          "Path does not seem to be a file.");
+          "Path does not seem to be a file.", thrower);


Hm, that's FileNotFound?

smarr · 2018-05-28T11:08:33Z

src/som/primitives/SystemPrims.java

    } catch (IOException e) {
-      PathPrims.signalIOException(e.getMessage());
+      PathPrims.signalIOException(e.getMessage(), thrower);


And here the same thrower node is IOException? seems fishy

smarr · 2018-05-28T13:26:45Z

@clementbera @daumayr think this here is a slightly nicer solution, because it avoids an interface: 52a0041

Also, I am going to change all thrower to use the exception name. Together with another change, the result reads nicely, for instance: argumentError.signal(...)

smarr · 2018-05-28T14:42:09Z

Ok, now I see why you might have decided for a lazy approach to initialize those exception nodes.
They need the module, but the module might not yet be loaded. Hmpf.

smarr · 2018-05-28T19:43:13Z

@daumayr @clementbera please review 8412ba6 carefully (see comment).
This might not be the smallest possible change. However, I consider it a useful exercise of some useful design choices and Truffle techniques. Delayed initialization can be achieved with node rewriting, which keeps the logic localized.

smarr · 2018-05-28T21:48:38Z

Ok, I am now happy with this. I did a bunch of other changes and clean ups.
Before merging, I am going to squash most of this, since the history is pretty uninteresting and I prefer a easily understandable set of changes.

Please review carefully.

daumayr · 2018-05-29T08:05:04Z

Looks good to me.
So replacing nodes is an option for lazy initialisation, I didn't know that.

…lingTruffleNode - added JUnits tests for Files Co-authored-by: Dominik Aumayr <[email protected]>

clementbera

I think there are some issues, sometimes nodes are initialized with insert(), sometimes not. I guess they should all have insert. Theres a single place where renaming was not done. I annotated all of them. I wanted to change, but for some reasons I have way too many conflicts when I pull.

clementbera · 2018-06-01T07:24:43Z

src/som/interpreter/nodes/InstantiationNode.java

    public ClassInstantiationNode(final MixinDefinition mixinDefinition) {
      super(mixinDefinition);
+      notAValueThrower = insert(ExceptionSignalingNode.createNotAValueNode(


This was not renamed (still named thrower)

clementbera · 2018-06-01T07:25:20Z

src/som/interpreter/nodes/InstantiationNode.java

@@ -88,6 +103,11 @@ public SClass instantiateClassWithNewClassFactory(final SObjectWithClass outerOb

    public ObjectLiteralInstantiationNode(final MixinDefinition mixinDefinition) {
      super(mixinDefinition);
+      notAValueThrower = ExceptionSignalingNode.createNotAValueNode(


This was not renamed (thrower) and it seems the insert() function is missing.

clementbera · 2018-06-01T07:26:58Z

src/som/interpreter/nodes/IsValueCheckNode.java

+    @SuppressWarnings("unchecked")
+    public ValueCheckNode initialize(final SourceSection sourceSection) {
+      super.initialize(sourceSection);
+      notAValue = ExceptionSignalingNode.createNotAValueNode(sourceSection);


It seems the insert() is missing.

clementbera · 2018-06-01T07:29:45Z

src/som/primitives/arrays/NewImmutableArrayNode.java

+  @SuppressWarnings("unchecked")
+  public NewImmutableArrayNode initialize(final SourceSection sourceSection) {
+    super.initialize(sourceSection);
+    notAValue = ExceptionSignalingNode.createNotAValueNode(sourceSection);


No insert()

smarr · 2018-06-01T08:07:05Z

I think there are some issues, sometimes nodes are initialized with insert(), sometimes not. I guess they should all have insert. Theres a single place where renaming was not done. I annotated all of them.

Inserts are needed when the nodes are added to a method after the whole method was construct.
Or after the first activation? I am not entirely sure. At one point, all child nodes are adopted ("inserted") automatically.

So, strictly speaking insert() is only needed after that point.
And yes, there might be inconsistencies in the code currently.

I wanted to change, but for some reasons I have way too many conflicts when I pull.

As mentioned before, I started squashing commits that do not add helpful information to the history and only make it harder to understand why and how things changed (fixups, in-development bugs, other changes that are only side-effects of the development, and not relevant for the end result).

Signed-off-by: Stefan Marr <[email protected]>

- pre-allocate commonly used exception-related symbols, and use instead of strings - rename execute to signal, and use directly - consistently use factory methods for exception nodes - reduce extensive verbosity, they are static methods, context is provided by the class name - change name of parameters: they are selectors, could be other things beside classes - restructure to enable lazy initialization on first activation - this is necessary to resolve modules which are loaded after parsing a method - also allocate the message send nodes lazily to avoid impact on startup performance - improve source section used for attribution to be the initializer - use node initializer methods consistently - remove ValuePrim, simply pass node instead - avoid `thrower` as name, instead use the type of the exception that’s going to be throw. Reads nicer: `argumentError.signal(…)` - remove unused code Signed-off-by: Stefan Marr <[email protected]>

Signed-off-by: Stefan Marr <[email protected]>

smarr · 2018-06-03T16:33:14Z

I finished the final cleanups, squashed the history where useful, and updated the issue title/description to explain the changes.

Note, there is another change hidden in here: the message send nodes for the exception handling are all allocated lazily now. There was some overhead on the startup performance, which is fixed by this.

Will merge once all tests/benchmarks passed.
I also included 3406c63 to add the performance note to SClass.isKindOf() (see #253).

smarr requested changes May 22, 2018

View reviewed changes

Repository owner deleted a comment May 23, 2018

clementbera mentioned this pull request May 23, 2018

Optimize Exception signaling #214

Closed

smarr requested changes May 28, 2018

View reviewed changes

smarr approved these changes May 28, 2018

View reviewed changes

smarr added enhancement Improves the implementation with something noteworthy language-design Not everything is in the spec, sometimes, we need to decide what's best. labels May 28, 2018

smarr added this to the v0.7.0 milestone May 28, 2018

smarr assigned clementbera May 28, 2018

Attempted to fix the issue with Dominik by creating an ExceptionSigna…

f6b6717

…lingTruffleNode - added JUnits tests for Files Co-authored-by: Dominik Aumayr <[email protected]>

smarr force-pushed the issue-214-optimize-exception-signaling branch 2 times, most recently from 65110a6 to a2983fd Compare May 29, 2018 21:37

clementbera commented Jun 1, 2018

View reviewed changes

Whitespace

a51471b

Signed-off-by: Stefan Marr <[email protected]>

smarr force-pushed the issue-214-optimize-exception-signaling branch from ad93285 to ef3e9d9 Compare June 3, 2018 15:11

smarr added 3 commits June 3, 2018 16:27

Update test data

6f7f2ea

Signed-off-by: Stefan Marr <[email protected]>

Added missing callerNeedsToBeOptimized to SClass.isKindOf

3406c63

Signed-off-by: Stefan Marr <[email protected]>

smarr force-pushed the issue-214-optimize-exception-signaling branch from ef3e9d9 to 3406c63 Compare June 3, 2018 16:12

smarr changed the title ~~Issue 214 optimize exception signaling~~ [Issue 214] Exception signaling without helper methods Jun 3, 2018

smarr merged commit 574a634 into smarr:dev Jun 3, 2018

smarr mentioned this pull request Jun 3, 2018

Errors when tracing DinerPhilosophers #253

Open

[Issue 214] Exception signaling without helper methods #251

[Issue 214] Exception signaling without helper methods #251

Conversation

clementbera commented May 22, 2018 • edited by smarr Loading

smarr commented May 22, 2018

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

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

smarr commented May 28, 2018

smarr commented May 28, 2018

smarr commented May 28, 2018

smarr commented May 28, 2018

daumayr commented May 29, 2018

clementbera 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

smarr commented Jun 1, 2018

smarr commented Jun 3, 2018

clementbera commented May 22, 2018 •

edited by smarr

Loading