Skip to content
Permalink
Browse files
[Truffle] Need to transfer because expensive construction of exceptio… 
…ns in Float#to_i.
  • Loading branch information
@nirvdrum
nirvdrum committed Dec 21, 2014
1 parent 97b1532 commit 6bd4b274d38e0bcfe3be7a73abf3f4f512e55127
Showing with 2 additions and 0 deletions.
  1. +2 −0 core/src/main/java/org/jruby/truffle/nodes/core/FloatNodes.java
2 core/src/main/java/org/jruby/truffle/nodes/core/FloatNodes.java
View file
@@ -628,10 +628,12 @@ public ToINode(ToINode prev) {
@Specialization
public Object toI(double value) {
if (Double.isInfinite(value)) {
CompilerDirectives.transferToInterpreter();
throw new RaiseException(getContext().getCoreLibrary().floatDomainError("Infinity", this));
}

if (Double.isNaN(value)) {
CompilerDirectives.transferToInterpreter();
throw new RaiseException(getContext().getCoreLibrary().floatDomainError("NaN", this));
}

6 comments on commit 6bd4b27

@thomaswue
Copy link
Contributor

@thomaswue thomaswue commented on 6bd4b27 Dec 21, 2014

Choose a reason for hiding this comment

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

Using transferToInterpreter here means that the cases where Infinity and NaN occurs will always be run in the interpreter (which might be the expected behavior). Alternatively, one can also use a branch profile to remove the branches from the fast path and optionally adding them back if they actually occur for the application.

@chrisseaton
Copy link
Contributor

@chrisseaton chrisseaton commented on 6bd4b27 Dec 21, 2014

Choose a reason for hiding this comment

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

The problem is that we can't compile the Infinity and NaN cases at the moment due to some string operations and other things involved in constructing the exceptions. Ideally those things should get fixed or get put behind TruffleBoundary. In the mean time we should annotate why we are transferring in each case. Perhaps we should define some aliases, for example transferBecauseCurrentlyUnableToCompile.

@thomaswue
Copy link
Contributor

@thomaswue thomaswue commented on 6bd4b27 Dec 21, 2014

Choose a reason for hiding this comment

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

OK. Can we make the constructions of those strings for exceptions lazy? Graal would be easily able to escape analyze a capturing closure. In this case it might be not so important, but there are always cases where programmers rely on exceptions for normal control flow ;).

@chrisseaton
Copy link
Contributor

@chrisseaton chrisseaton commented on 6bd4b27 Dec 21, 2014

Choose a reason for hiding this comment

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

Yes you're right - we do want all these cases to be on the fast path. One problem at the moment that we have is that in order to construct an exception we need to construct a backtrace object, and in order to do that we need to walk the frame stack to find all the call nodes and their source sections. I believe that requires a full deoptimization anyway. That's why I hadn't tackled this problem yet. Using the backtrace object is pretty common and isn't a corner case we can't ignore. For example the RubySpec runner we are using makes extensive use of it.

I believe there is some research on lazy deoptimization. So where we'd normally deoptimize to get a call node for a frame we have some kind of lazy object that gets forced either when we ask for it - or when the frame gets unwound so it wouldn't exist any more if you did force it.

@thomaswue
Copy link
Contributor

@thomaswue thomaswue commented on 6bd4b27 Dec 21, 2014

Choose a reason for hiding this comment

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

Maybe we can avoid the construction of the backtrace if Graal sees it is unused. Anyway, let's revisit fast exceptions at a later point in time. In the meantime putting the work of constructing the exception behind a TruffleBoundary seems the right thing to do.

@chrisseaton
Copy link
Contributor

@chrisseaton chrisseaton commented on 6bd4b27 Dec 21, 2014

Choose a reason for hiding this comment

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

Ok - we'll be doing a big pass through the code after we've merged a few more branches after the 0.6 release, and we'll check on all the exceptions behind transfers then.

Please sign in to comment.