Recent Truffle DSL changes.

[Stefan Marr]

Hi Christian:

Trying to figure out what the issue could be, I am wondering what the idea behind the replaceHelper logic and the difference between adoptHelper(.) and adoptUnadoptedHelper(.) is.

Somehow, there seem to be situations in which the resulting tree contains nodes for which the parent link has not been set.

I am also wondering, whether the approach of synchronizing on the level of the rewrite operations itself is reliable enough or whether the synchronization should happen one level up, i.e., the specialization logic might need to ensure that each node is only specialized exactly once.
Otherwise, two threads could end up specializing the tree multiple times for the same node, racing for the replace. The thread that wins the race for the replace will continue executing a sub tree that is going to be removed from main tree once the second thread specialized the node. I assume, this could lead to all kind of strange and undesirable situations.

Does any of that make sense, or am I missing something?

Thanks
Stefan


On 18 Sep 2014, at 00:05, Christian Humer <christian.humer at gmail.com> wrote:

> Hi Stefan,
> 
> Regarding the thread-safety issues. Yes polymorphic rewrites are not yet thread-safe. But they were also not thread-safe in the old version. So you might have been just lucky.
> However it is straight forward to make them thread-safe. You just have to wrap every replace in DSLShare into a closure and call atomic on the node that is currently in the AST.
> I've attached a patch. Let me know if that works for you. So I can push it tomorrow. (Had no time to verify it yet) 
> Also be careful with the rewriting of function inline caches so that replaces are always atomic. Also important is that you are not moving one used AST node from one place to another. Instead copy it, remove the old and insert the copy.
> 
> 
> - Christian Humer
> 
> On Wed, Sep 17, 2014 at 8:57 PM, Stefan Marr <java at stefan-marr.de> wrote:
> Hi Christian:
> 
> Now I am running into issues.
> I belief your new rewriting logic in the generated code for polymorphic nodes is not completely thread safe.
> 
> Specifically, I am running into issues that a node is supposed to be rewritten, but not actually adopted yet.
> 
> In case you might have a hunch, let me know. Will try to have a look later this week.
> 
> 
> To reproduce, you’ll need the thread-support branch and start a benchmark with at least 2 threads:
> 
> git clone --recursive -b thread-support https://github.com/SOM-st/TruffleSOM.git
> cd TruffleSOM
> ant
> ./som.sh -cp Smalltalk Examples/Benchmarks/BenchmarkHarness.som Dispatch 10 0 10 2
> 
> Best regards
> Stefan
> 
> 
> On 16 Sep 2014, at 23:57, Stefan Marr <java at stefan-marr.de> wrote:
> 
> > Hi Christian:
> >
> > On 11 Aug 2014, at 19:59, Christian Humer <christian.humer at gmail.com> wrote:
> >
> >> I just pushed some new features to the DSL for the upcoming Truffle
> >> release. Please note that some of the changes may break existing Truffle
> >> interpreters. Guest languages that don't use Truffle DSL are not affected
> >> by this push.
> >
> > I finally found the time to update Graal.
> > Looks good. Didn’t had any major issues. Performance seems to remain more or less the same.
> >
> > Only got two nits. The first one is that the generated code does have one simple issue with the code style, which can be solved by initializing the method body unconditionally (see patch below).
> > The second thing is a simple typo in one of the error messages.
> >
> > Best regards
> > Stefan
> >
> > diff --git a/graal/com.oracle.truffle.dsl.processor/src/com/oracle/truffle/dsl/processor/generator/NodeCodeGenerator.java b/graal/com.oracle.truffle.dsl.processor/src/com/oracle/truffle/dsl/processor/generator/NodeCodeGenerator.java
> > index 0a38640..c3bc5eb 100644
> > --- a/graal/com.oracle.truffle.dsl.processor/src/com/oracle/truffle/dsl/processor/generator/NodeCodeGenerator.java
> > +++ b/graal/com.oracle.truffle.dsl.processor/src/com/oracle/truffle/dsl/processor/generator/NodeCodeGenerator.java
> > @@ -906,9 +906,9 @@ public class NodeCodeGenerator extends AbstractCompilationUnitFactory<NodeData>
> >             CodeExecutableElement method = new CodeExecutableElement(modifiers(PUBLIC), context.getType(void.class), "updateTypes0");
> >             method.getParameters().add(new CodeVariableElement(classArray, "types"));
> >
> > -            if (getModel().isPolymorphic()) {
> > -                CodeTreeBuilder builder = method.createBuilder();
> > +            CodeTreeBuilder builder = method.createBuilder();
> >
> > +            if (getModel().isPolymorphic()) {
> >                 int index = 0;
> >                 for (NodeExecutionData execution : getModel().getNode().getChildExecutions()) {
> >                     String fieldName = polymorphicTypeName(execution);
> > diff --git a/graal/com.oracle.truffle.dsl.processor/src/com/oracle/truffle/dsl/processor/parser/NodeParser.java b/graal/com.oracle.truffle.dsl.processor/src/com/oracle/truffle/dsl/processor/parser/NodeParser.java
> > index 202815e..516dc18 100644
> > --- a/graal/com.oracle.truffle.dsl.processor/src/com/oracle/truffle/dsl/processor/parser/NodeParser.java
> > +++ b/graal/com.oracle.truffle.dsl.processor/src/com/oracle/truffle/dsl/processor/parser/NodeParser.java
> > @@ -650,7 +650,7 @@ public class NodeParser extends AbstractParser<NodeData> {
> >             SpecializationData next = i + 1 < specializations.size() ? specializations.get(i + 1) : null;
> >
> >             if (!cur.isContainedBy(next)) {
> > -                next.addError("This specialiation is not a valid exceptional rewrite target for %s. To fix this make %s compatible to %s or remove the exceptional rewrite.",
> > +                next.addError("This specialization is not a valid exceptional rewrite target for %s. To fix this make %s compatible to %s or remove the exceptional rewrite.",
> >                                 cur.createReferenceName(), next != null ? next.createReferenceName() : "-", cur.createReferenceName());
> >                 continue;
> >             }
> > --
> > 1.9.1
> >
> >
> >
> >
> > --
> > Stefan Marr
> > INRIA Lille - Nord Europe
> > http://stefan-marr.de/research/
> >
> >
> >
> 
> --
> Stefan Marr
> INRIA Lille - Nord Europe
> http://stefan-marr.de/research/
> 
> 
> 
> 
> <dsl_thread_safety.diff>

-- 
Stefan Marr
INRIA Lille - Nord Europe
http://stefan-marr.de/research/