Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Truffle] Extracted Kernel#eval's to_str coercion to a new node.
Browse files Browse the repository at this point in the history
@nirvdrum
nirvdrum committed Feb 6, 2015
1 parent f8392f9 commit d935095
Showing 2 changed files with 105 additions and 48 deletions.
87 changes: 87 additions & 0 deletions truffle/src/main/java/org/jruby/truffle/nodes/coerce/ToStrNode.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* Copyright (c) 2015 Oracle and/or its affiliates. All rights reserved. This
* code is released under a tri EPL/GPL/LGPL license. You can use it,
* redistribute it and/or modify it under the terms of the:
*
* Eclipse Public License version 1.0
* GNU General Public License version 2
* GNU Lesser General Public License version 2.1
*/

package org.jruby.truffle.nodes.coerce;

import com.oracle.truffle.api.dsl.NodeChild;
import com.oracle.truffle.api.dsl.Specialization;
import com.oracle.truffle.api.frame.VirtualFrame;
import com.oracle.truffle.api.source.SourceSection;
import org.jruby.truffle.nodes.RubyNode;
import org.jruby.truffle.nodes.dispatch.CallDispatchHeadNode;
import org.jruby.truffle.nodes.dispatch.DispatchHeadNodeFactory;
import org.jruby.truffle.runtime.RubyContext;
import org.jruby.truffle.runtime.control.RaiseException;
import org.jruby.truffle.runtime.core.RubyString;

@NodeChild(value = "child", type = RubyNode.class)
public abstract class ToStrNode extends RubyNode {

@Child private CallDispatchHeadNode toStr;

public ToStrNode(RubyContext context, SourceSection sourceSection) {
super(context, sourceSection);
toStr = DispatchHeadNodeFactory.createMethodCall(context);
}

public ToStrNode(ToStrNode prev) {
super(prev);
toStr = prev.toStr;
}

@Specialization
public RubyString doRubyString(RubyString string) {

This comment has been minimized.

Sign in to view

Copy link
@chrisseaton

chrisseaton Feb 6, 2015

Contributor

Can we call this cast or something? If we call all our execute methods doType it makes the stack harder to read (you still have the class name, but it's still good I think).

This comment has been minimized.

Sign in to view

Copy link
@nirvdrum

nirvdrum Feb 6, 2015

Author Contributor

Sure. I was just following the BooleanCastNode. I think I prefer "coerce" though. I suppose that's a bigger discussion about what constitutes coercing and what constitutes casting.
return string;
}

@Specialization(guards = "!isRubyString")
public RubyString doObject(VirtualFrame frame, Object object) {
notDesignedForCompilation();

final Object coerced;

try {
coerced = toStr.call(frame, object, "to_str", null);
} catch (RaiseException e) {
if (e.getRubyException().getLogicalClass() == getContext().getCoreLibrary().getNoMethodErrorClass()) {
throw new RaiseException(
getContext().getCoreLibrary().typeError(
String.format("no implicit conversion of %s into String", getContext().getCoreLibrary().getLogicalClass(object).getName()),
this));
} else {
throw e;
}
}

if (coerced instanceof RubyString) {
return (RubyString) coerced;
} else {
final String uncoercedClassName = getContext().getCoreLibrary().getLogicalClass(object).getName();

throw new RaiseException(
getContext().getCoreLibrary().typeError(
String.format("can't convert %s to String (%s#to_str gives %s)",
uncoercedClassName,
uncoercedClassName,
getContext().getCoreLibrary().getLogicalClass(coerced).getName()),
this));
}
}

@Override
public abstract RubyString executeRubyString(VirtualFrame frame);

public abstract RubyString executeRubyString(VirtualFrame frame, Object object);

@Override
public final Object execute(VirtualFrame frame) {
return executeRubyString(frame);
}
}
66 changes: 18 additions & 48 deletions truffle/src/main/java/org/jruby/truffle/nodes/core/KernelNodes.java
View file
Original file line number Diff line number Diff line change
@@ -12,6 +12,9 @@
import com.oracle.truffle.api.CompilerDirectives;
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
import com.oracle.truffle.api.Truffle;
import com.oracle.truffle.api.dsl.CreateCast;
import com.oracle.truffle.api.dsl.NodeChild;
import com.oracle.truffle.api.dsl.NodeChildren;
import com.oracle.truffle.api.dsl.Specialization;
import com.oracle.truffle.api.frame.*;
import com.oracle.truffle.api.nodes.UnexpectedResultException;
@@ -25,6 +28,8 @@
import org.jruby.truffle.nodes.cast.BooleanCastNodeFactory;
import org.jruby.truffle.nodes.cast.NumericToFloatNode;
import org.jruby.truffle.nodes.cast.NumericToFloatNodeFactory;
import org.jruby.truffle.nodes.coerce.ToStrNode;
import org.jruby.truffle.nodes.coerce.ToStrNodeFactory;
import org.jruby.truffle.nodes.control.WhileNode;
import org.jruby.truffle.nodes.core.KernelNodesFactory.SameOrEqualNodeFactory;
import org.jruby.truffle.nodes.dispatch.*;
@@ -513,19 +518,27 @@ public Object dup(VirtualFrame frame, RubyBasicObject self) {
}

@CoreMethod(names = "eval", isModuleFunction = true, required = 1, optional = 3)
public abstract static class EvalNode extends CoreMethodNode {
@NodeChildren({
@NodeChild(value = "source", type = RubyNode.class),

This comment has been minimized.

Sign in to view

Copy link
@eregon

eregon Feb 6, 2015

Member

I think the type does not need to be specified here, but the DSL infers it from the Node type I would guess.
@NodeChild(value = "binding", type = RubyNode.class),
@NodeChild(value = "filename", type = RubyNode.class),
@NodeChild(value = "lineNumber", type = RubyNode.class)
})
public abstract static class EvalNode extends RubyNode {

@Child private CallDispatchHeadNode toStr;
@Child private BindingNode bindingNode;

public EvalNode(RubyContext context, SourceSection sourceSection) {
super(context, sourceSection);
toStr = DispatchHeadNodeFactory.createMethodCall(context);
}

public EvalNode(EvalNode prev) {
super(prev);
toStr = prev.toStr;
}

@CreateCast("source") public RubyNode coerceSourceToString(RubyNode source) {
return ToStrNodeFactory.create(getContext(), getSourceSection(), source);
}

protected RubyBinding getCallerBinding(VirtualFrame frame) {
@@ -574,57 +587,14 @@ public Object eval(RubyString source, RubyBinding binding, RubyString filename,
return getContext().eval(source.getBytes(), binding, false, this);
}

@Specialization(guards = "!isRubyString(arguments[0])")
public Object eval(VirtualFrame frame, RubyBasicObject object, UndefinedPlaceholder binding, UndefinedPlaceholder filename, UndefinedPlaceholder lineNumber) {
notDesignedForCompilation();

return evalCoerced(frame, object, getCallerBinding(frame), true, filename, lineNumber);
}

@Specialization(guards = "!isRubyString(arguments[0])")
public Object eval(VirtualFrame frame, RubyBasicObject object, RubyBinding binding, UndefinedPlaceholder filename, UndefinedPlaceholder lineNumber) {
notDesignedForCompilation();

return evalCoerced(frame, object, binding, false, filename, lineNumber);
}

@Specialization(guards = "!isRubyBinding(arguments[1])")
public Object eval(RubyBasicObject source, RubyBasicObject badBinding, UndefinedPlaceholder filename, UndefinedPlaceholder lineNumber) {
@Specialization(guards = "!isRubyBinding(binding)")
public Object eval(RubyString source, RubyBasicObject badBinding, UndefinedPlaceholder filename, UndefinedPlaceholder lineNumber) {
throw new RaiseException(
getContext().getCoreLibrary().typeError(
String.format("wrong argument type %s (expected binding)",
badBinding.getLogicalClass().getName()),
this));
}

private Object evalCoerced(VirtualFrame frame, RubyBasicObject object, RubyBinding binding, boolean ownScopeForAssignments, UndefinedPlaceholder filename, UndefinedPlaceholder lineNumber) {
Object coerced;

try {
coerced = toStr.call(frame, object, "to_str", null);
} catch (RaiseException e) {
if (e.getRubyException().getLogicalClass() == getContext().getCoreLibrary().getNoMethodErrorClass()) {
throw new RaiseException(
getContext().getCoreLibrary().typeError(
String.format("no implicit conversion of %s into String", object.getLogicalClass().getName()),
this));
} else {
throw e;
}
}

if (coerced instanceof RubyString) {
return getContext().eval(((RubyString) coerced).getBytes(), binding, ownScopeForAssignments, this);
} else {
throw new RaiseException(
getContext().getCoreLibrary().typeError(
String.format("can't convert %s to String (%s#to_str gives %s)",
object.getLogicalClass().getName(),
object.getLogicalClass().getName(),
getContext().getCoreLibrary().getLogicalClass(coerced).getName()),
this));
}
}
}

@CoreMethod(names = "exec", isModuleFunction = true, required = 1, argumentsAsArray = true)

5 comments on commit d935095

@nirvdrum
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrisseaton @eregon Can you review this please?

I basically extracted the String coercion logic in Kernel#eval to a new node. I replaced its usage in Kernel#eval with a cast node so the eval source is always a RubyString. I could change that to be a child node instead. There are pros and cons to each and I don't see a clear winner. Although going this way allows us to remove a couple specializations.

@chrisseaton
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. The other option is to handle RubyString in eval, and then call a fallback method when that isn't the case that does the coercion in Ruby, where it's easier to write, and then calls the method again.

@nirvdrum
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 think that's the same as the child node case. I can go that route, too. It just means more specializations in eval. But it does drop the overhead in interpreter mode for the common case of a String source already being supplied.

@eregon
Copy link
Member

@eregon eregon commented on d935095 Feb 6, 2015

Choose a reason for hiding this comment

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

I would rather use a Rubinius-like primitive if we want to do the coercion in Ruby:

def eval(source, binding, *)
   primitive :vm_eval
   eval(convert_to_str(source), convert_to_binding_or_undef(bidning), *)
end

Hard to say what's best here.

@eregon
Copy link
Member

@eregon eregon commented on d935095 Feb 6, 2015

Choose a reason for hiding this comment

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

@nirvdrum Looks good, let's merge it!

Please sign in to comment.