Skip to content

Commit

Permalink
Showing 1 changed file with 11 additions and 2 deletions.
13 changes: 11 additions & 2 deletions truffle/src/main/java/org/jruby/truffle/core/array/ArrayNodes.java
Original file line number Diff line number Diff line change
@@ -63,6 +63,7 @@
import org.jruby.truffle.language.RubyGuards;
import org.jruby.truffle.language.RubyNode;
import org.jruby.truffle.language.RubyRootNode;
import org.jruby.truffle.language.SnippetNode;
import org.jruby.truffle.language.arguments.MissingArgumentBehavior;
import org.jruby.truffle.language.arguments.ReadPreArgumentNode;
import org.jruby.truffle.language.arguments.RubyArguments;
@@ -3972,9 +3973,17 @@ public DynamicObject sortVeryShortObject(VirtualFrame frame, DynamicObject array
return createArray(getContext(), store, size);
}

public static final String SNIPPET = "sorted = dup; Rubinius.privately { sorted.isort_block!(0, right, block) }; sorted";
public static final String RIGHT = "right";
public static final String BLOCK = "block";

@Specialization(guards = { "!isNullArray(array)" })
public Object sortUsingRubinius(VirtualFrame frame, DynamicObject array, DynamicObject block) {
return ruby("sorted = dup; Rubinius.privately { sorted.isort_block!(0, right, block) }; sorted", "right", getSize(array), "block", block);
public Object sortUsingRubinius(
VirtualFrame frame,
DynamicObject array,
DynamicObject block,
@Cached("new(SNIPPET, RIGHT, BLOCK)") SnippetNode snippet) {
return snippet.execute(frame, getSize(array), block);
}

@Specialization(guards = { "!isNullArray(array)", "!isSmall(array)" })

11 comments on commit f6a2f67

@chrisseaton
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jruby/truffle here's better example of using the new SnippetNode.

@pitr-ch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, I think I liked the createSnippet() version more, because the arguments were not substituted with the constants.

@chrisseaton
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does everyone else think?

Maybe we could write:

@Cached(
    "new($1, $2, $3)",
    "sorted = dup; Rubinius.privately { sorted.isort_block!(0, right, block) }; sorted",
    "right", "block")`

@eregon
Copy link
Member

@eregon eregon commented on f6a2f67 Apr 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current version or a createSnippet helper both look fine to me. I think the $ would be too much magic.

@nirvdrum
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree with Petr. But I generally find it annoying to track down where the code in a @cached fragment lives. The old solution looked like it could be adopted into a pattern of sorts and then I'd always know where to look as long as the pattern is followed.

@chrisseaton
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to always put the final fields next to the specialisation, not at the top of the class. The reason I don't like Petr's preferred option is that it adds another method when we just got the ability to not have to do that (with no context or section)!

@nirvdrum
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly enough one way or the other. I'm just concerned our runtime is getting to be too stringly typed.

Sorry, something went wrong.

@pitr-ch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

        public static final String SNIPPET = "sorted = dup; Rubinius.privately { sorted.isort_block!(0, right, block) }; sorted";
        public static final String RIGHT = "right";
        public static final String BLOCK = "block";

        @Specialization(guards = { "!isNullArray(array)" })
        public Object sortUsingRubinius1(
                VirtualFrame frame,
                DynamicObject array,
                DynamicObject block,
                @Cached("new(SNIPPET, RIGHT, BLOCK)") SnippetNode snippet) {
            return snippet.execute(frame, getSize(array), block);
        }

        @Specialization(guards = { "!isNullArray(array)" })
        public Object sortUsingRubinius2(
                VirtualFrame frame,
                DynamicObject array,
                DynamicObject block,
                @Cached("new(\"sorted = dup; Rubinius.privately { sorted.isort_block!(0, right, block) }; sorted\", \"right\", \"block\")") SnippetNode snippet) {
            return snippet.execute(frame, getSize(array), block);
        }

        @interface CachedSnippet {
            String[] value();
        }

        @Specialization(guards = { "!isNullArray(array)" })
        public Object sortUsingRubinius3(
                VirtualFrame frame,
                DynamicObject array,
                DynamicObject block,
                @CachedSnippet({
                        "sorted = dup; Rubinius.privately { sorted.isort_block!(0, right, block) }; sorted",
                        "right",
                        "block" }) SnippetNode snippet) {
            return snippet.execute(frame, getSize(array), block);
        }

        @Specialization(guards = { "!isNullArray(array)" })
        public Object sortUsingRubinius4(
                VirtualFrame frame,
                DynamicObject array,
                DynamicObject block,
                @Cached("createSnippet()") SnippetNode snippet) {
            return snippet.execute(frame, getSize(array), block);
        }

        protected SnippetNode createSnippet() {
            return new SnippetNode(
                    "sorted = dup; Rubinius.privately { sorted.isort_block!(0, right, block) }; sorted",
                    "right",
                    "block");
        }

        public static class InnerEvalNode extends RubyBaseNode {

            @Child SnippetNode snippetNode;

            public Object execute(
                    VirtualFrame frame,
                    String expression,
                    String[] argumentNames,
                    Object... arguments) {

                if (snippetNode == null) {
                    CompilerDirectives.transferToInterpreter();
                    snippetNode = insert(new SnippetNode(expression, argumentNames));
                }

                if (CompilerDirectives.inInterpreter()) {
                    // TODO (pitr-ch 19-Apr-2016): check the expression and argumentNames are matching
                }

                return snippetNode.execute(frame, arguments);
            }
        }

        @Specialization(guards = { "!isNullArray(array)" })
        public Object sortUsingRubinius5(
                VirtualFrame frame,
                DynamicObject array,
                DynamicObject block,
                @Cached("new()") InnerEvalNode eval) {
            return eval.execute(
                    frame,
                    "sorted = dup; Rubinius.privately { sorted.isort_block!(0, right, block) }; sorted",
                    new String[]{ "right", "block" },
                    getSize(array),
                    block);
        }

// 6
@Cached({
    "new($1, $2, $3)",
    "sorted = dup; Rubinius.privately { sorted.isort_block!(0, right, block) }; sorted",
    "right", "block"})

Considering all the options:

  1. -argument substitution, +works
  2. -quote escaping, -requires truffle update
  3. -requires probably complicated truffle update
  4. -extra method, +works
  5. +works, -extra node
  6. -magic, -requires truffle update

the 5. option seems best to me.

@eregon
Copy link
Member

@eregon eregon commented on f6a2f67 Apr 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was also thinking to something like 5, where essentially we just add snippet. before ruby(... but then the constraint that the code and arguments must be fixed is slightly unclear.
Another alternative is to standardize argument names to for instance argN, then we only need a constant for the code (similar to 1).

@pitr-ch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it's unclear but it can be checked to catch mis-use (when compiled and used correctly the check should be eliminated I think, otherwise it could be checked only on interpreted path). 1 with argN seems good too (I still slightly prefer 5 though.)

@chrisseaton
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could live with 5. Everyone happy with that?

Please sign in to comment.