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

Optimize Promises performance #61

Closed
wants to merge 16 commits into from
Closed

Optimize Promises performance #61

wants to merge 16 commits into from

Conversation

@TvdW
Copy link

TvdW commented Jun 13, 2017

Hi Stevan,

I've optimized Promises for performance. Here's the pull request (updated a bit after your comments on #60). It's almost backwards compatible, but I've made the done() function safe (according to the docs it was unsafe with known bad side-effects) and updated docs accordingly. According to a search through all CPAN modules using Promises, this behavior was not used anywhere but the Promises tests.

Quick benchmark :

# OLD
Benchmark: running one, two for at least 10 CPU seconds...
       one: 10 wallclock secs (10.42 usr +  0.00 sys = 10.42 CPU) @ 8939.35/s (n=93148)
       two: 11 wallclock secs (10.89 usr +  0.00 sys = 10.89 CPU) @ 3057.21/s (n=33293)

# NEW
Benchmark: running one, two for at least 10 CPU seconds...
       one: 11 wallclock secs (10.59 usr +  0.00 sys = 10.59 CPU) @ 28733.81/s (n=304291)
       two: 10 wallclock secs (10.09 usr +  0.00 sys = 10.09 CPU) @ 10273.84/s (n=103663)

And of course the corresponding code :

use v5.18;
use warnings;
use Benchmark qw/timethese/;
use Promises qw/collect deferred/;

sub a_promise {
    my $deferred= deferred;
    $deferred->resolve(1,2,3,4,5);
    return $deferred->promise;
}

timethese(-10, {
    one => sub {
        my $have_result;
        a_promise()->then(sub { a_promise(); })->then(sub { $have_result= 1; });
        die unless $have_result;
    },
    two => sub {
        my $i;
        a_promise()->then(sub {
            if (++$i == 5) {
                return;
            } else {
                a_promise()->then(__SUB__);
            }
        });
        die unless $i == 5;
    },
});
TvdW added 12 commits Jun 1, 2017
This is *mostly* feature-complete. Specifically, done() no longer
propagates die() when running under certain event loops, as this was
event loop dependent and thus behavior one should not rely on
(especially as a module author who is not supposed to care about the
backend in use.)

This is not yet documented, addressing that later. The relevant test
cases also still fail, the others all seem good.
As of my rewrite, done() doesn't swallow exceptions anymore. This
behavior was bad anyway, as it depended on the chosen eventloop, and was
flagged as 'unsafe' in the documentation.

Although there was a test for this behavior, it shouldn't stay. Module
authors couldn't depend on it (they don't get to choose the backend),
and promise chains would get broken if you did this. Calling done()
twice on the promise (or once plus a then()), and dying from the
callback, would cause the other to not be invoked.

It's gone now. So here's a patch updating the tests to reflect the new
behavior.
If we don't do this, we can end up never resolving/rejecting our
promises, which is a bad thing.
With the 'Default' backend it's possible that we execute callbacks
immediately. That's a great way to trigger weird cases, so it's best to
delay the invocation of callbacks as long as we can.
We don't have to create a new promise when we return things, we can also
just return them if the promise wasn't rejected.
@yanick

This comment has been minimized.

Copy link
Collaborator

yanick commented Aug 19, 2017

Beginning to review this. It might.. take a while. :-)

@yanick

This comment has been minimized.

Copy link
Collaborator

yanick commented Sep 18, 2017

I began to merge the most no-brainish parts of the PR. I have some points to discuss for the beefier. More details in a few seconds. :-)

}

# Now for all the convenience methods
sub done { &then; (); }

This comment has been minimized.

Copy link
@yanick

yanick Sep 18, 2017

Collaborator

I'm not sure about this. Basically (and very literally), you're turning done into an alias for then. But the thing is, with then errors are supposed to be caught by else and the such down the chain. If we do the same thing with done, they'll just be... swallowed into the void. That's not good.

I am tempted to say that if you want the behavior of containing errors and pushing them down the chain, then use then. If you want your explosions uncontained, then use done.

And yeah, done will have different behaviors depending on the backend. I guess what we can do, in addition of having that well documented, is to have Promises issue warnings on that kind of potentially divergent behaviors if a flag is passed. Something like

use Promises qw/ deferred /, warnings => [ 'backend_dependent' ];

deferred()->then(sub{ ... })->done(sub{ });  # warns "Promise's 'done' behaviour is dependent on the backed"

This comment has been minimized.

Copy link
@TvdW

TvdW Sep 20, 2017

Author

I'm sure about this :)

It's

  • unpredictable. behavior depends on the backend, which means library authors shouldn't ever use it
  • violating the promises spec. my $promise= deferred;$promise->done(sub{die;});$promise->then(sub{say "hi";});$promise->resolve; -- the "hi" must be written but the code doesn't
  • dangerous. if I use done() on a promise returned by a library, I may break that library which did not expect $deferred->resolve(@result) to die.
  • unused on CPAN. I scanned all the users of Promises and nobody relies on done() propagating errors. as they shouldn't.

This comment has been minimized.

Copy link
@TvdW

TvdW Sep 20, 2017

Author

As I explained it to a colleague: as soon as you've thrown an error from a single done() callback, you can't use Promises anymore.

I don't think that's good :)

This comment has been minimized.

Copy link
@yanick

yanick Sep 20, 2017

Collaborator

If the exceptions were all voluntary, I would agree. But the thing is, you don't always die because you wanted to. Sometimes, you'll have

$promise->done(sub { play_with_unicorns() } );

and for some reason, the unicorns will rebel and stab you in the chest. If that happen, the error will be swallowed without any mean to check for it, which will make for exceptionally horrendous debugging sessions. It'd be like the evil dimension twin of Moo's old behavior that made warnings fatal.

So I'm afraid I'm standing rather firmly on that one. Moreover considering that, if it really holds dear to the programmer, the possibility to get that behavior by always using then is already there.

This comment has been minimized.

Copy link
@TvdW

TvdW Sep 20, 2017

Author

So you'd rather stick with broken/unsafe behavior than include a backwards incompatbile fix for a code path that's probably not used by anything, and that nobody but Promises maintainers can reason about?

I don't see the logic there. done() is a function that allows people to shoot themselves in the foot without them realizing it. There's nothing good about it, why keep it?

This comment has been minimized.

Copy link
@yanick

yanick Sep 20, 2017

Collaborator

As mentioned in the first comment, I'm not averse to issue warnings and considering deprecating the method. I'm strongly against changing it in a way that will silently swallow exceptions.

This comment has been minimized.

Copy link
@clintongormley

clintongormley Sep 20, 2017

Contributor

It's been a long time since I added done, but it was added with good reason: to avoid stack overflows from too much recursion, which are easy to trigger with Promises. Making it a synonym for then() is the wrong solution.

This comment has been minimized.

This comment has been minimized.

Copy link
@TvdW

TvdW Sep 20, 2017

Author

It's not a synonym for then(), the new implementation of done() still behaves in a way that helps with recursion.

How about making it warn in case it gets rejected? Would that be a good middle ground?

This comment has been minimized.

Copy link
@TvdW

TvdW Sep 20, 2017

Author

After reading further on the JS implementation, it seems it exists for logging purposes, not to change the flow of code. Amending my reimplementation of done() with a warning would align the implementation of done here, with the JS variant you linked to.

TvdW added 4 commits May 31, 2018
It can result in us scheduling thousands of notify callbacks, all
without anything to do.
When there are no events to be handled, epoll just waits until the
timeout expires. Under EV and most other event loops, the lowest
possible timeout is 1ms. postpone{} is implemented with a timer, which
doesn't count as an event, so we end up waiting 1ms every time we
postpone{} and there's no network traffic.

Let's create fake traffic instead by using a pipe. In my tests this is
100x faster in almost all cases.
This is awesome: $promise->then(sub{next}) -- watch what happens.
Got to avoid leaking scope into weird places...
ehuelsmann added a commit to ehuelsmann/promises-perl that referenced this pull request May 16, 2019
ehuelsmann added a commit to ehuelsmann/promises-perl that referenced this pull request May 16, 2019
ehuelsmann added a commit to ehuelsmann/promises-perl that referenced this pull request May 16, 2019
ehuelsmann added a commit to ehuelsmann/promises-perl that referenced this pull request Jun 12, 2019
@yanick yanick closed this in 78ee22b Jun 15, 2019
ehuelsmann added a commit to ehuelsmann/promises-perl that referenced this pull request Jun 17, 2019
yanick added a commit that referenced this pull request Jun 22, 2019
 * In similar vain to PR #61, optimize _notify_backend (for IO::Async). (GH#85, Erik Huelsmann)

Fixes #85
yanick added a commit that referenced this pull request Jun 22, 2019
  [ DOCUMENTATION ]
    - Fixed pod error as reported by CPANTS. (GH#83, Mohammad S Anwar)

  [ ENHANCEMENTS ]
    - In similar vain to PR #61, optimize _notify_backend (for IO::Async).
      (GH#85, Erik Huelsmann)

  [ STATISTICS ]
    - code churn: 3 files changed, 31 insertions(+), 9 deletions(-)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.