New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Suspensions to Skulpt #286

Merged
merged 5 commits into from Oct 20, 2014

Conversation

Projects
None yet
8 participants
@meredydd
Contributor

meredydd commented Sep 21, 2014

This pull request adds a new feature to Skulpt: "Suspensions", which allow
Python execution to be paused and resumed. This sort of functionality has
been discussed before (h/t @LeszekSwirski issue #150), and the general
consensus is that it was desirable, so here's a working implementation
with tests.


Suspensions are continuations, generated on demand by returning an instance
of Sk.misceval.Suspension from a function. They allow Python execution to be
suspended and subsequently resumed, allowing simulation of blocking I/O,
time-slicing to keep web pages responsive, and even multi-threading.

Normally, a suspension is initiated by a callee (Javascript) function,
but there is also a 'debugging' option (passed to Sk.configure()) which
causes the compiler to generate a suspension before each statement.
As each suspension also captures all stack frames and local variables,
this can be used to implement a single-step debugger. An optional
breakpoints() callback (also passed to Sk.configure()) allows the user
to dynamically filter which of these suspensions actually happen.

Suspensions have a 'data' property, which is an object indicating the reason
for the suspension (and under what circumstances it may be resumed).
'data.type' is a string; current suggested values are:

'timer':   Resume after a delay. 'data.delay' is the delay in seconds
           before resuming. time.sleep() emits 'timer' suspensions.
'promise': Resume when the Javascript Promise 'data.promise' resolves or
           yields an error.
'debug':   A suspension cause by the 'debugging' option (see above)

Suspensions also have an 'optional' property. If set to true, this suspension
may be resumed immediately if it is not convenient to wait. 'debug'
suspensions have 'optional' set, so they are ignored rather than causing
errors in non-suspendable call stacks (see below).

Example: --------------------------------------------------------------------

Here is some suspension-aware Javascript code that calls a Python function
that might suspend, and gives the result as a Javascript promise:

function callAsync(pyFunction) {
   var p = Promise.defer();

   var handleResponse = function(r) {

     if (r instanceof Sk.misceval.Suspension) {

       var doResume = function() {
         var newR = r.resume();
         handleResponse(newR);
       }

       switch (r.data.type) {
         case 'timer':
           setTimeout(doResume, r.data.delay*1000);
           break;

         case 'promise':
           r.data.promise.then(doResume, doResume);
           break;

         default:
           if (r.optional) {
             doResume(); // immediately
           } else {
             p.reject("Unknown suspension type " + r.data.type);
           }
       }

     } else { // It's not a suspension, just a regular return value

       p.resolve(r);
     }
   };

   var r = Sk.misceval.callsimOrSuspend(pyFunction);
   handleResponse(r);
}

(Note, this is an example: A real implementation would catch exceptions
thrown by callsim(pyFunction) or r.resume() so they don't get lost in the
event loop. This code has been omitted for clarity.)

Implementation notes: -------------------------------------------------------

  • Not every Javascript calling context can handle getting a suspension
    instead of a return value. There are many awkward cases within the Skulpt
    codebase, let alone existing users of the library. Therefore, uses of
    Sk.misceval.callsim()/call()/apply() do not support suspensions, and will
    throw a SuspensionError if their callee tries to suspend non-optionally.
    (Likewise, suspending part-way through a class declaration will produce
    an error.)

    Other APIs which call into Python, such as import and Sk.abstr.iternext(),
    now have an extra parameter, 'canSuspend'. If false or undefined, they
    will throw a SuspensionError if their callee suspends non-optionally. If
    true, they may return a Suspension instead of a result.

    If a Suspension with the 'optional' flag is returned in a non-suspendable
    context, its resume() method will be called immediately rather than
    causing an error.

  • Suspensions save the call stack as they are processed. This provides a
    Python stack trace of every suspension. This could be used to provide
    stack traces on exceptions, which is currently a missing feature.

  • Likewise, suspensions would be a natural way of implementing generators.
    The current generator implementation is quite limited (it does not support
    varargs or keyword args) and not quite correct (it does not preserve
    temporaries between calls - see below), so would benefit from unification.

  • Suspensions would also be a good way of implementing timeouts, as well as
    keeping the browser responsive during long computations. I have not
    changed the existing timeout code, which still throws errors. A
    suspension-based timeout should first return optional suspensions (in case
    the timeout triggers on a non-suspendable stack), and then, after a grace
    period, issue a non-optional suspension that will terminate a
    non-suspendable stack.

Reliability and testing notes: ----------------------------------------------

  • Deliberate suspensions are tested by the (newly implemented) time.sleep()
    function, which is exercised by the new tests t544.py and t555.py.

  • We test that a wide variety of generated code is robust to being suspended
    by running the entire test suite in 'debugging' mode (see above). This
    causes suspensions and resumptions at every statement boundary, giving us
    good confidence that any feature exercised by the test suite is robust to
    suspension. "./skulpt.py testdebug" is the command for this.

    Of course, the test suite must also be run in normal mode, to verify that
    it works when not suspending at every statement boundary.

Performance notes:

  • Essential overhead in the fast case (ie code that does not suspend) is
    kept quite low, at two conditionals per function call (one by the caller,
    to check whether a call completed or suspended, and one by the callee,
    to check whether this is a normal call or a suspension being resumed).
    Given the number of conditionals and nested function calls in
    Sk.misceval.call/callsim/apply, this is probably negligible.
  • There is additional implementation-dependent overhead (ie overhead that
    can be whittled down if it proves too much, and would not require global
    changes to implementations strategy to mitigate). I have not attacked
    these too aggressively, as the indications are that the performance hit
    already isn't that bad (~5% on the test suite on my machine, but of course
    we'd need bigger benchmarks to say for sure). Still, here are some
    pointers for future improvement:
    • Sk.misceval.call/apply and friends are now wrappers around
      applyOrSuspend, with an additional check for suspensions (to throw an
      error)
    • Each function call creates a new block for "after this function
      returns" (this is where we resume to if that function call suspends),
      and jumps to it via the normal continue-based mechanism. With more
      invasive modification to the block generator, we could use switch-case-
      fallthrough to remove this penalty for ordinary function returns.
    • Any temporary that might be required to persist across any suspension
      in a scope (ie any function call) is saved on every suspension. This
      is conservative but correct (unlike existing generator support, which
      just breaks if you try something like x = str((yield y)), as the
      temporary used to look up 'str' is not preserved). However, this does
      impede the compiler's ability to infer variable lifetimes. This
      could be mitigated by generating separate save and resume code for
      each suspension site, but that again requires intrusive modification
      to the block system.

@meredydd meredydd force-pushed the meredydd:suspension-pr branch 4 times, most recently from 4b50b82 to ae1b7b6 Sep 21, 2014

meredydd added some commits Sep 19, 2014

Add support for suspensions.
Suspensions are continuations, generated on demand by returning an instance
of Sk.misceval.Suspension from a function. They allow Python execution to be
suspended and subsequently resumed, allowing simulation of blocking I/O,
time-slicing to keep web pages responsive, and even multi-threading.

Normally, a suspension is initiated by a callee (Javascript) function,
but there is also a 'debugging' option (passed to Sk.configure()) which
causes the compiler to generate a suspension before each statement.
As each suspension also captures all stack frames and local variables,
this can be used to implement a single-step debugger. An optional
breakpoints() callback (also passed to Sk.configure()) allows the user
to dynamically filter which of these suspensions actually happen.

Suspensions have a 'data' property, which is an object indicating the reason
for the suspension (and under what circumstances it may be resumed).
'data.type' is a string; current suggested values are:

    'timer':   Resume after a delay. 'data.delay' is the delay in seconds
               before resuming. time.sleep() emits 'timer' suspensions.
    'promise': Resume when the Javascript Promise 'data.promise' resolves or
               yields an error.
    'debug':   A suspension cause by the 'debugging' option (see above)

Suspensions also have an 'optional' property. If set to true, this suspension
may be resumed immediately if it is not convenient to wait. 'debug'
suspensions have 'optional' set, so they are ignored rather than causing
errors in non-suspendable call stacks (see below).

Example: --------------------------------------------------------------------

Here is some suspension-aware Javascript code that calls a Python function
that might suspend, and gives the result as a Javascript promise:

    function callAsync(pyFunction) {
       var p = Promise.defer();

       var handleResponse = function(r) {

         if (r instanceof Sk.misceval.Suspension) {

           var doResume = function() {
             var newR = r.resume();
             handleResponse(newR);
           }

           switch (r.data.type) {
             case 'timer':
               setTimeout(doResume, r.data.delay*1000);
               break;

             case 'promise':
               r.data.promise.then(doResume, doResume);
               break;

             default:
               if (r.optional) {
                 doResume(); // immediately
               } else {
                 p.reject("Unknown suspension type " + r.data.type);
               }
           }

         } else { // It's not a suspension, just a regular return value

           p.resolve(r);
         }
       };

       var r = Sk.misceval.callsimOrSuspend(pyFunction);
       handleResponse(r);
    }

(Note, this is an example: A real implementation would catch exceptions
thrown by callsim(pyFunction) or r.resume() so they don't get lost in the
event loop. This code has been omitted for clarity.)

Implementation notes: -------------------------------------------------------

 * Not every Javascript calling context can handle getting a suspension
   instead of a return value. There are many awkward cases within the Skulpt
   codebase, let alone existing users of the library. Therefore, uses of
   Sk.misceval.callsim()/call()/apply() do not support suspensions, and will
   throw a SuspensionError if their callee tries to suspend non-optionally.
   (Likewise, suspending part-way through a class declaration will produce
   an error.)

   Other APIs which call into Python, such as import and Sk.abstr.iternext(),
   now have an extra parameter, 'canSuspend'. If false or undefined, they
   will throw a SuspensionError if their callee suspends non-optionally. If
   true, they may return a Suspension instead of a result.

   If a Suspension with the 'optional' flag is returned in a non-suspendable
   context, its resume() method will be called immediately rather than
   causing an error.

 * Suspensions save the call stack as they are processed. This provides a
   Python stack trace of every suspension. This could be used to provide
   stack traces on exceptions, which is currently a missing feature.

 * Likewise, suspensions would be a natural way of implementing generators.
   The current generator implementation is quite limited (it does not support
   varargs or keyword args) and not quite correct (it does not preserve
   temporaries between calls - see below), so would benefit from unification.

 * Suspensions would also be a good way of implementing timeouts, as well as
   keeping the browser responsive during long computations. I have not
   changed the existing timeout code, which still throws errors. A
   suspension-based timeout should first return optional suspensions (in case
   the timeout triggers on a non-suspendable stack), and then, after a grace
   period, issue a non-optional suspension that will terminate a
   non-suspendable stack.

Reliability and testing notes: ----------------------------------------------

 * Deliberate suspensions are tested by the (newly implemented) time.sleep()
   function, which is exercised by the new tests t544.py and t555.py.

 * We test that a wide variety of generated code is robust to being suspended
   by running the entire test suite in 'debugging' mode (see above). This
   causes suspensions and resumptions at every statement boundary, giving us
   good confidence that any feature exercised by the test suite is robust to
   suspension.

   Of course, the test suite must also be run in normal mode, to verify that
   it works when *not* suspending at every statement boundary.

Performance notes:

 * Essential overhead in the fast case (ie code that does not suspend) is
   kept quite low, at two conditionals per function call (one by the caller,
   to check whether a call completed or suspended, and one by the callee,
   to check whether this is a normal call or a suspension being resumed).
   Given the number of conditionals and nested function calls in
   Sk.misceval.call/callsim/apply, this is probably negligible.

 * There is additional implementation-dependent overhead (ie overhead that
   can be whittled down if it proves too much, and would not require global
   changes to implementations strategy to mitigate). I have not attacked
   these too aggressively, as the indications are that the performance hit
   already isn't that bad (~5% on the test suite on my machine, but of course
   we'd need bigger benchmarks to say for sure). Still, here are some
   pointers for future improvement:

    - Sk.misceval.call/apply and friends are now wrappers around
      applyOrSuspend, with an additional check for suspensions (to throw an
      error)

    - Each function call creates a new block for "after this function
      returns" (this is where we resume to if that function call suspends),
      and jumps to it via the normal continue-based mechanism. With more
      invasive modification to the block generator, we could use switch-case-
      fallthrough to remove this penalty for ordinary function returns.

    - Any temporary that might be required to persist across *any* suspension
      in a scope (ie any function call) is saved on *every* suspension. This
      is conservative but correct (unlike existing generator support, which
      just breaks if you try something like x = str((yield y)), as the
      temporary used to look up 'str' is not preserved). However, this does
      impede the compiler's ability to infer variable lifetimes. This
      could be mitigated by generating separate save and resume code for
      each suspension site, but that again requires intrusive modification
      to the block system.
skulpt.py and test.js changes to test in debug mode (without a breakp…
…oint filter, this suspends before every statement)

@meredydd meredydd force-pushed the meredydd:suspension-pr branch from ae1b7b6 to e8e39d4 Sep 21, 2014

@bnmnetp

This comment has been minimized.

Contributor

bnmnetp commented Sep 22, 2014

Thanks for the PR, I look forward to looking at it more carefully and getting feedback from the rest of the community over the coming days.

@LeszekSwirski

This comment has been minimized.

Contributor

LeszekSwirski commented Sep 22, 2014

Looks like a great patch @meredydd, I'm impressed how minimal it is considering the changes to control flow it introduces.

Reading through it, would I be correct in understanding that the current sleep test, as written, doesn't actually sleep, and it's up to the user to handle the suspension correctly? That is, that your example callAsync is what in #150 I called a trampoline?

If that's the case, it seems that the user has to be aware of the different suspension types, checking Suspension.data.type. I assume this is a design decision to allow debug promises to be checked by the user? If so, it seems that debug and promise are the only two types of Suspension that can happen (timer can be implemented in terms of promises, as can timeouts), and that the former is always optional while the latter never is. Perhaps this means that you could replace Suspension.data.type and Suspension.optional with just Suspension.isDebug, and assume that everything else is a Promise.

For me, this would be less scary in terms of forwards compatibility, as it seems that any new Suspension types that are added in the current framework almost always have to be optional, for the sake of backwards compatibility. I admit however that it may also be a bit shortsighted.

@LeszekSwirski

This comment has been minimized.

Contributor

LeszekSwirski commented Sep 22, 2014

Additionally, I would update the sample code (that is, amend the commit message) with handling of promises that return values, presumably by making doResume take a parameter and forward it to r.resume().

Since it looks like the example usage is mildly non-trivial,especially with correct error handling, it would also be nice to have a function in the library which takes a potential suspension (like the return of import*) and "just does the right thing", presumably returning a Promise of the return value.

@ebertmi

This comment has been minimized.

Contributor

ebertmi commented Sep 22, 2014

Seems to be pretty interesting. I was thinking about using it for my simulation of an arduino board, that is programmable with skulpt.

@meredydd

This comment has been minimized.

Contributor

meredydd commented Sep 22, 2014

Thanks, @LeszekSwirski.

You're right; 'timer' should be implemented in terms of 'promise'. (I initially tried to avoid "everything is a promise" because the d8 environment used for the Skulpt tests doesn't support promises by default, but a little Googling has led me to the --harmony_promises option. The absence of setTimeout() might still be an issue, though.)

However, I'm not sure that should be the only sort of suspension supported, or that handling of everything except debug suspensions should be sealed inside the library. Debug suspensions are already not the only optional suspension to account for - for example, in the commit notes, I talk about how a suspension-based timeout and time-slicing system should generate optional suspensions to avoid unnecessarily crashing non-suspendable call stacks. And although we might provide useful defaults, handling of those will very much depend on the user's resource-allocation and -sharing model.

More generally, the ability to generate (almost) arbitrary continuations and generally screw with the call stack is a powerful tool that should be exposed to whoever wants it. How about implementing a library callAsync() function, which optionally takes a list of every type of suspension you want to handle manually? Anything you're not explicitly will be handled automatically, and a SuspensionError thrown if we see something that isn't optional and we don't know how to deal with? (I'd namespace the suspension types, then, so we'd have 'Sk.promise' etc)

(Re return values: promise return values should be returned in the susp.data object, and picked out by the inner resume() function, not passed as a parameter. The Suspension returned by the suspending JS function is not the same object as the Suspension you get back from applyOrSuspend() - it gets wrapped at each level with a new Suspension representing that stack frame, and the inner one placed in susp.child. Only 'type', 'optional' and 'data' are copied. If we wanted to support parameters to resume(), we would have to thread them right back up the call stack with apply(arguments), which seems a little gross. That said, there should absolutely be a library function for implementing a classic continuation API like this.)

@LeszekSwirski

This comment has been minimized.

Contributor

LeszekSwirski commented Sep 22, 2014

@meredydd

Fair enough with the d8 environment, but tests not supporting library functionality are an issue to fix in the test suite, not the library. Maybe you could mock some Promise objects and setTimeout functions directly into the tests?

You make a good point about other optional suspensions. I started writing an argument about how they're really a manifestation of the same type of suspension as debug suspensions, but following that train of thought led me to optional suspensions having an associated type, and that sounds a lot like what's there already. The only thing that's missing is, as you say, a library function for handling them that allows a user to manually inject themselves into suspension handling (probably an object mapping type to function), and has sensible defaults otherwise.

Copying return values into data makes sense, adding an additional test (probably with a mock Promise object) to demonstrate this would be great.

@meredydd

This comment has been minimized.

Contributor

meredydd commented Sep 22, 2014

Sorry, I was a bit cryptic there - turns out that d8 actually does support promises if you give it the right arguments. So yes, I'll rev that (probably this evening). I expect I can mock/work around setTimeout().

Anyone else have more feedback?

@albertjan

This comment has been minimized.

Member

albertjan commented Sep 22, 2014

Awsome work! 😄 I do think we need something like this to get it to work in older browsers.

@albertjan

This comment has been minimized.

Member

albertjan commented Sep 22, 2014

Oh yeah and I don't think the commit message is the best 😄 place to have documentation. It might be a good idea to make a page for the website demonstrating the behaviour. With documentation.

@meredydd

This comment has been minimized.

Contributor

meredydd commented Sep 22, 2014

Thanks @albertjan. Do you think bundling a Promse polyfill/implementation is a good idea? I'd have thought it cleaner merely to list that as a dependency, although I can see the argument for not giving downstream users a nasty surprise.

How does one add a page to the website? The commit message was aimed more at the pull request (LKML-patch-style...I'm still new to this Github thing ;) ), but I'll rev it into a pair of text files in the repo (one for users, one for hacking).

@albertjan

This comment has been minimized.

Member

albertjan commented Sep 22, 2014

@meredydd We have to, we cannot axe everyone with a browser that doesn't support promises. I would like to make sure we only do things with promises that are also supported by the polyfill. As you can see here only a little over half of the users have native promises. I really like you used the standards compliant promises for this, and I think the polyfill is probably going to a solid fall back scenario.

I very much like that you think we are as organised as the linux kernel guys. And the information in the commit message won't be lost. But as a consumer of skulpt I think having a working webpage with documentation is much easier to find and to understand than the commit message. So it doesn't have much to do with github or anything, more with ease in finding the docs. That being said any sort of documentation is better than none, and as you probably know skulpt lacks some in that department. So it was more of an idea than a requirement.

I really like the amount of avenues of new features this opens up. We could do breakpoints!! And we could probably also use this for the execution limit, if we incorporate that into the suspensions we would shave off some of the overhead, since we're checking those also every other line.

@meredydd

This comment has been minimized.

Contributor

meredydd commented Sep 22, 2014

Oh, the polyfill is certainly a requirement. It's a question of whether we bundle a snapshot of some Promise library into our code, or publish it as a dependency requirement (eg as a Bower dependency).

I'm glad you're as excited about this as I am :)

Upgrade d8 to v3.25 with Promise support
Change some tests that appear to rely on d8's floating point sharing the
same errors (!) as CPython's, which they don't in this version.
@albertjan

This comment has been minimized.

Member

albertjan commented Sep 23, 2014

@meredydd How do you think we could go about writing a good performance test for this I don't assume it adds any significant work in the compiler.

@albertjan

This comment has been minimized.

Member

albertjan commented Sep 23, 2014

@meredydd It appears my idea to add the polyfill library to the codebase might have been a bad one. At least to add it before the closure compiler. The idea mostly spawned from the fact that we put some effort into making skulpt dependency free not so long ago.

Thanks for compiling new v8 versions, I hope my notes were helpful 😄.

I was just about to look into stripping the execution limit stuff from the compiler and using the suspensions to limit the execution time. Thats why I asked about the perf tests. But it appears I'm out of time for tonight. 😄 I'll try and have a look at it tomorrow.

@meredydd

This comment has been minimized.

Contributor

meredydd commented Sep 23, 2014

Yeah; I've been bashing my head against the wall for the last half-hour trying to get es6-promise through Closure. (Annoyingly, it seems that they actually use Closure for their build, but I can't find the magic incantation that makes it ignore the grungy undeclared-variable magic necessary for polyfilling.)

Incorporate feedback from pull request.
* Utility functions to present asynchronous execution as a Promise
  (now used by all test scripts)

* Bundle a Promise polyfill library

* Add doc/suspensions.txt

@meredydd meredydd force-pushed the meredydd:suspension-pr branch from d2c7bcb to 2502b6d Sep 23, 2014

@meredydd

This comment has been minimized.

Contributor

meredydd commented Sep 23, 2014

Ouch. It took me the best part of two hours to wrestle a hacked version of that polyfill through Closure. Please don't make me do it again :)

The v8 compilation notes were useful, primarily in reassuring me that rebuilding it really was that simple. The hardest part was borrowing @daviesian's Mac.

It would be great to see the execution limit reworked with suspensions. I think the change would be pretty small - not so much stripping it as replacing that code block with one which suspends. Given that the checks already only happen on block boundaries, and no temporaries need saving, it should be short and sweet. The new asyncToPromise() function should make the bottom-end simpler, too.

@LeszekSwirski

This comment has been minimized.

Contributor

LeszekSwirski commented Sep 23, 2014

@meredydd, I'm sure you had a good reason for this as always, but why does asyncToPromise take a () -> value|Suspension instead of just a value|Suspension (i.e. directly take r as an argument)?

@LeszekSwirski

This comment has been minimized.

Contributor

LeszekSwirski commented Sep 23, 2014

Heroic effort on the polyfill btw.

@meredydd

This comment has been minimized.

Contributor

meredydd commented Sep 23, 2014

@LeszekSwirski because that way, there's only one way an error can come out. If you wrote:

hypotheticalAsyncToPromise(Sk.importMain("xyz", true, true)).then(...)

...then any exception thrown during the import of the "xyz" module will either be thrown from this line (if it happens before it suspends) or come out as a rejection of the promise (if it happens after a suspension). Putting the initial call inside a function lets us catch and report both immediate and delayed exceptions through the promise.

@LeszekSwirski

This comment has been minimized.

Contributor

LeszekSwirski commented Sep 24, 2014

There we go, good reason.

@bnmnetp

This comment has been minimized.

Contributor

bnmnetp commented Sep 29, 2014

Indeed I can see that it does, even under Yosemite.   

But there is still the mystery of the command line arguments.  Even on a Mavericks mac today, I tried, the —harmony_promises option is not supported, although as you said the Promise object is there.  On the mac doing ./d8m —help does not even list —harmony_promises as an option.   I guess maybe since it is there anyway.

Is this just a case of nobody running the tests through skulpt.py on a mac?

-- 
Brad Miller
Sent with Airmail

On September 29, 2014 at 1:45:38 PM, Albert-Jan Nijburg (notifications@github.com) wrote:

the v8 on mac has Promises even without the flag.


Reply to this email directly or view it on GitHub.

@albertjan

This comment has been minimized.

Member

albertjan commented Sep 29, 2014

We can omit it on mac. On my other mac I'm also seeing that it doesnt have that flag. Maybe its because i built d8 on the first one.

Sent from my iPhone

On 29 sep. 2014, at 21:35, "Brad Miller" notifications@github.com wrote:

Indeed I can see that it does, even under Yosemite.

But there is still the mystery of the command line arguments. Even on a Mavericks mac today, I tried, the —harmony_promises option is not supported, although as you said the Promise object is there. On the mac doing ./d8m —help does not even list —harmony_promises as an option. I guess maybe since it is there anyway.

Is this just a case of nobody running the tests through skulpt.py on a mac?

Brad Miller
Sent with Airmail

On September 29, 2014 at 1:45:38 PM, Albert-Jan Nijburg (notifications@github.com) wrote:

the v8 on mac has Promises even without the flag.


Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub.

@LeszekSwirski

This comment has been minimized.

Contributor

LeszekSwirski commented Sep 29, 2014

FWIW, the --harmony_promises flag was removed in v8/v8@1df5820 (and the dummy flag that replaced it was removed in v8/v8@a03d646).

* the null suspension handler is:
*
* function handler(susp) {
* return new Promise(resolve, reject) {

This comment has been minimized.

@LeszekSwirski

LeszekSwirski Sep 30, 2014

Contributor

Should be

new Promise(function(resolve, reject) {
@LeszekSwirski

This comment has been minimized.

Contributor

LeszekSwirski commented Sep 30, 2014

@meredydd On the subject of potential performance effects, have you investigated the effect of inlining $saveSuspension? I don't have a new enough build of d8 on my Windows machine to do any testing right now, but I suspect that the clojure+name lookup of $saveSuspension might have a non-zero cost, and I'm not sure that it'd get inlined (or even if v8 JITs evaled code at all).

@meredydd

This comment has been minimized.

Contributor

meredydd commented Sep 30, 2014

$saveSuspension is explicitly the slow path - I'm more concerned about the case where it isn't called than when it is. (Also, it gets quite big in a non-trivial function, thanks to all the saved temporaries, so I wouldn't fancy inlining it for every function call.)

That said, according to that linked page, every generated Skulpt function is left entirely un-optimised because it contains a try/catch block. This is a useful target for future performance enhancement, but it's out of scope for this PR. It gives no information on whether functions defined in evaled code can be optimised.

Thanks for the typo fix in that comment.

@LeszekSwirski

This comment has been minimized.

Contributor

LeszekSwirski commented Sep 30, 2014

It's the slow path, but also the common path when in debug mode. I shouldn't think that inlining it is difficult given that you're generating the code anyway, and I assume that we're hoping for the JIT to inline the code anyway, so we may as well do it ourselves.

I misread the link above, I thought it said any function that was eval'ed, not any function that contains an eval.

@meredydd

This comment has been minimized.

Contributor

meredydd commented Sep 30, 2014

Debug mode is a compile-time switch, so I'm not opposed to inlining in that
mode.

I wouldn't be desperate to do it speculatively, though. There's a good
chance the optimiser will do something sensible with $saveSuspension (it's
statically-provably local, and only ever called in a tail context, so it
might even just optimise down to a jump). I'd want to test both ways, and
we can't do that if the try-catch block is stopping the optimiser dead. On
the upside, this would be some pretty low-hanging fruit...

@LeszekSwirski

This comment has been minimized.

Contributor

LeszekSwirski commented Oct 3, 2014

Sounds sensible.

I've had a chance to have a play with this, and it's great -- debugging mode made it stunningly easy to highlight the currently executing line, something I've wanted to have for a while.

@bnmnetp

This comment has been minimized.

Contributor

bnmnetp commented Oct 4, 2014

I have built a test version of both my books using this branch, and have run through all of the examples. Everything seems to work as usual.

@rixner have you had a chance to look at this?

I would really like to accept this, as I think it opens up many great options for the future development of Skulpt.

  • a new version of the turtle module
  • integration with the online python tutor
  • a debugger
  • simulated I/O -- loading images from anywhere for example.
  • I would also guess an improved REPL could come out of this

Brad

@bnmnetp

This comment has been minimized.

Contributor

bnmnetp commented Oct 6, 2014

Trying to understand this more. I tried the following in doc/index.html

import time
print "hello"
time.sleep(5)
print "goodbye"

What I got was:

    hello
SuspensionError: Module "__main__" suspended or blocked during load, and it was loaded somewhere that does not permit this on line 4

Am I missing something, does the runit function need to be modified somehow?

@meredydd

This comment has been minimized.

Contributor

meredydd commented Oct 6, 2014

Yes, it does. If you drill into where that javascript calls importMain() or
similar, you'll have to:

  1. Add the extra parameter to that call to set "canSuspend" to true (see
    import.js for the new method signature)

  2. Wrap it in a call to suspToAsync, as with the example in
    doc/Suspensions. This will return a Promise that resolves when the code
    finishes running, or is rejected when it throws an exception.

There's a simple example of this usage in the Javascript generated by
skulpt.py for the "skulpt.py run" command. There's a more complex example
in test.js, which uses promises to wait until one test has finished
executing before starting the next.

Does that help?

@LeszekSwirski

This comment has been minimized.

Contributor

LeszekSwirski commented Oct 6, 2014

Sk.misceval.asyncToPromise rather than suspToAsync.

@meredydd

This comment has been minimized.

Contributor

meredydd commented Oct 6, 2014

Thanks; was on my phone

@bnmnetp

This comment has been minimized.

Contributor

bnmnetp commented Oct 6, 2014

OK, that did it.

Sorry, I did not see the suspensions.txt file with the documentation.

Once I accept the PR I'll update the gist, so that we have that documented in our simple example on the main page.

Am I correct in assuming that the benchmarks reported by @albertjan where done with canSuspend == true?

@meredydd

This comment has been minimized.

Contributor

meredydd commented Oct 6, 2014

Passing the canSuspend parameter to a function like importMain() doesn't affect the compiled code. It only affects the behaviour of the function if/when it calls Python code that suspend. Namely, if canSuspend isn't true, importMain() throws an exception instead of returning a Suspension object. This ensures that existing code that does not handle suspensions (eg your original code) fails loudly and traceably rather than subtly. The performance hit comes from testing the return value of each function call for whether it is a Suspension, which must be done whether your calling context supports suspensions or not.

Sk.misceval.applyAsync = function (suspHandlers, func, kwdict, varargseq, kws, args) {
return Sk.misceval.asyncToPromise(function() {
return Sk.misceval.applyOrSuspend(func, kwdict, varargseq, kws, args);
});

This comment has been minimized.

@LeszekSwirski

LeszekSwirski Oct 7, 2014

Contributor

suspHandlers is lost here.

This comment has been minimized.

@meredydd

meredydd Oct 7, 2014

Contributor

Yeah, @IanDavies just pointed that out too :)

We also have a bug with suspending generators if they're explicitly called
via the next() method. Expect a revision tomorrow.

This comment has been minimized.

@IanDavies

IanDavies Oct 7, 2014

I don't believe I'm the Ian Davies you're looking for :)

On 7 Oct 2014, at 18:05, Meredydd Luff notifications@github.com wrote:

In src/misceval.js:

  •            } catch(e) {
    
  •                reject(e);
    
  •            }
    
  •        })(r);
    
  •    } catch (e) {
    
  •        reject(e);
    
  •    }
    
  • });
    +};
    +goog.exportSymbol("Sk.misceval.asyncToPromise", Sk.misceval.asyncToPromise);

+Sk.misceval.applyAsync = function (suspHandlers, func, kwdict, varargseq, kws, args) {

  • return Sk.misceval.asyncToPromise(function() {
  •    return Sk.misceval.applyOrSuspend(func, kwdict, varargseq, kws, args);
    
  • });
    Yeah, @IanDavies just pointed that out too :) We also have a bug with suspending generators if they're explicitly called via the next() method. Expect a revision tomorrow.

    Reply to this email directly or view it on GitHub.

@meredydd meredydd force-pushed the meredydd:suspension-pr branch from e4b78a5 to 1e1bb01 Oct 8, 2014

@meredydd

This comment has been minimized.

Contributor

meredydd commented Oct 9, 2014

A (hopefully) final tweak - fix the bug pointed out by @LeszekSwirski, and allow suspension in constructors as well as the use of the .next() call on generators.

I think this is ready to roll now. I'm happy to submit further expansions (eg properly async imports) as a separate PR.

@albertjan

This comment has been minimized.

Member

albertjan commented Oct 10, 2014

@meredydd How difficult do you think it will be to merge this with #201?

@LeszekSwirski

This comment has been minimized.

Contributor

LeszekSwirski commented Oct 10, 2014

@albertjan, should be fairly trivial, the suspension code pretty much only affects function calls, other control flow logic should (famous last words) stay the same.

@LeszekSwirski

This comment has been minimized.

Contributor

LeszekSwirski commented Oct 10, 2014

Yup, quick and simple merge (the only conflicts were test names) seems to work fine, modulo finally/with not being entirely implemented:

http://www.cl.cam.ac.uk/~ls426/skulpt/

@albertjan

This comment has been minimized.

Member

albertjan commented Oct 10, 2014

Awesome thanks :)

@bnmnetp

This comment has been minimized.

Contributor

bnmnetp commented Oct 19, 2014

Ok, everyone. I've tagged the current master at 0.9.8. Hearing no objections, I plan to accept this (with my own small modifications to skulpt.py to remove the --harmony options and add the Promise object to jshintrc file.) first thing tomorrow.

Brad

@albertjan

This comment has been minimized.

Member

albertjan commented Oct 19, 2014

👍

@bnmnetp bnmnetp merged commit 1e1bb01 into skulpt:master Oct 20, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@meredydd meredydd deleted the meredydd:suspension-pr branch Mar 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment