Skip to content
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

Async & Performance: optimize gen-runner in chapter 4 #1210

Open
getify opened this issue Jan 7, 2018 · 13 comments
Open

Async & Performance: optimize gen-runner in chapter 4 #1210

getify opened this issue Jan 7, 2018 · 13 comments
Assignees

Comments

@getify
Copy link
Owner

getify commented Jan 7, 2018

In the promise-aware gen runner, to prevent the unnecessary extra tick at the beginning, change:

return Promise.resolve().then(
   function handleNext(value){ .. }
);

...to:

return Promise.resolve(
   (function handleNext(value){ .. })()
);
@getify getify self-assigned this Jan 7, 2018
@getify
Copy link
Owner Author

getify commented Jan 9, 2018

Code also shown in chapter 4 of "ES6 & Beyond"

@benjamingr
Copy link

benjamingr commented Jan 16, 2018

When I wrote that suggestion the deferring of a tick was intentional for clarity of code - but I don't have strong feelings about it.

@getify
Copy link
Owner Author

getify commented Jan 16, 2018

The reason I don't like the extra tick is because an equivalent async function doesn't wait a tick to start, it runs its first part immediately.

@benjamingr
Copy link

@getify you're right :) I didn't know that would be the case when I first wrote that pump code.

Another part of the fact it's wrapped in a then provides throw safety which would need to be a try/catch - although I trust you know better than me what people might find easier to understand in the book.

@getify
Copy link
Owner Author

getify commented Jan 16, 2018

the fact it's wrapped in a then provides throw safety which would need to be a try/catch

Ahh, you're absolutely right, totally forgot that was one of the motivations. That should have been commented. ;-)

OK, so we need this, huh?:

return Promise.resolve(
   (function handleNext(value){
      try {
         var next = it.next(value);
         // ..
      }
      catch (err) {
         return Promise.reject(err);
      }
   })()
);

@benjamingr
Copy link

Pretty much, yeah, although then you can probably add a Promise.resolve on the value if the iterator is done and drop the Promise.resolve around everything.

@getify
Copy link
Owner Author

getify commented Jan 16, 2018

the other (minor) downside is that this new try..catch is run (unnecessarily) on every subsequent step, not just the first step...it's unnecessary becasue the subsequent steps already have their own try..catch via the then(..). :/

@getify
Copy link
Owner Author

getify commented Jan 16, 2018

FYI: I've previously adapted this gen-runner to use in another of my projects, CAF.

So just tweaked it based on our discussions here: https://github.com/getify/CAF/blob/94c60e2cdba714680f770ca04bd3fe2e5201b830/src/caf.src.js#L58-L98

Thoughts?

In particular, I'm curious/pondering if it's better (or different/broken) to move that outer catch(..) { .. } clause from line 94 up to line 65, and then just leave the return .. on line 66 outside of the try..catch? What do you think?

@getify
Copy link
Owner Author

getify commented Jan 16, 2018

(edit): I thought moving the catch broke my tests, but it didn't. So I've made that change now: https://github.com/getify/CAF/blob/c084ebfeea8eabe28676f105700eb8575776024b/src/caf.src.js#L58-L98

Thoughts?

@benjamingr
Copy link

CAF looks really interesting, it's a shame we don't have proper cancel token support from the platform. I see you .return on the generator on cancellation which is what we did at bluebird and the reasonable thing to do IMO.

I would probably write it with an async function which would be really cute usage IMO:

function _runner(gen, ...args) {
  const it = gen.apply(this, args);
  async function result() {
    let done = false, value = undefined;
    while(!done) {
      try {
        ({done, value}) = it.next(await value);
      } catch (e) {
        it.throw(e);
      }
    }
    return value;
  }
  return {it, result() }
}

Although I'm not sure of its performance characteristics. What about the perhaps simpler:

function _runner(gen, ...args) {
  const it = gen.apply(this, args);
  function result(oldValue) {
    let {done, value} = it.next(oldValue);
    return Promise.resolve(value).then(result, it.throw.bind(it));
  }
  return {it, (async () => result())() } // the `async` lambda is to handle `throw`s
}

@getify
Copy link
Owner Author

getify commented Jan 16, 2018

My main reason for not making it an async function is that my baseline target is ES6 support rather than ES2017 support. I don't like people to have to transpile my libraries if it's avoidable.

@getify
Copy link
Owner Author

getify commented Jan 16, 2018

it's a shame we don't have proper cancel token support from the platform.

We do have the web platform's standard of AbortController, which CAF adheres to.

@jfet97
Copy link

jfet97 commented Aug 30, 2018

@getify What about that?

function run(gen, ...args) {

    let it = gen(...args);
    let next;

    try {
        next = it.next();
        return handleValue(next);
    } catch (err) {
        return Promise.reject(err);
    }


    function handleErr(err) {
        return Promise.resolve(it.throw(err)).then(handleValue);
    }


    function handleValue(next) {
        if (!next.done) {
            return Promise.resolve(next.value).then(nextStep, handleErr);
        } else {
            return Promise.resolve(next.value);
        }
    }

    function nextStep(value) {
        next = it.next(value);
        return handleValue(next);
    }
}

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

No branches or pull requests

3 participants