Skip to content

Conversation

dom96
Copy link
Contributor

@dom96 dom96 commented Dec 22, 2017

Yet another way to fix #6100, replaces #6915 and #6614 (I guess as well?).

The speeds are as follows for the test added in this PR:

I think there is more to #6614 than just #6100 though. Thoughts welcome.

@yglukhov
Copy link
Member

As far as I understand, this PR handles the already completed yielded futures, thats where the speed improvement comes from. #6614 handles a different case when (deferred) future completion results in numerous subsequent future completions. Also it takes future completion responsibility away from dispatcher, and makes it compatible with JS, btw =). So I'd say both PRs need to be merged, it's just a coincidence they solve similar problems in their own ways :D

@dom96
Copy link
Contributor Author

dom96 commented Dec 23, 2017

Now the question is, which is preferable? This or #6915?

Personally I of course prefer this PR. I think it's better to generate less code.

@yglukhov
Copy link
Member

yglukhov commented Dec 23, 2017

I personally prefer to avoid adding complexity to macros, so I would vote for this one.

Now as I'm thinking about #6614... After your PR is merged, I can't imagine a real life scenario when a future completion would recursively trigger lots of other completions. So I guess now the future.complete can just call the callback directly and #6614 can be simplified even more. Can you confirm that?

@dom96
Copy link
Contributor Author

dom96 commented Dec 23, 2017

Now as I'm thinking about #6614... After your PR is merged, I can't imagine a real life scenario when a future completion would recursively trigger lots of other completions. So I guess now the future.complete call can just call the callback directly and #6614 can be simplified even more. Can you confirm that?

Heh, so we would be going back full circle.

Unfortunately I'm honestly not sure whether completions could still be triggered recursively. But I would be willing to take the risk and if it turns out to be a problem get a nice test case for it.

@Araq
Copy link
Member

Araq commented Dec 27, 2017

Breaks tests/async/tfuturestream.nim

@dom96 dom96 merged commit 2c905f5 into devel Jan 10, 2018
@narimiran narimiran deleted the fixes/6100 branch October 11, 2018 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

newest asyncdispatch recursion
3 participants