Skip to content
This repository has been archived by the owner on Mar 26, 2019. It is now read-only.

Asyncdispatch2 move. #26

Merged
merged 4 commits into from
Jun 18, 2018
Merged

Asyncdispatch2 move. #26

merged 4 commits into from
Jun 18, 2018

Conversation

cheatfate
Copy link
Contributor

Move nim-eth-p2p to use asyncdispatch2.
Fix some warnings and debug echoes in rlpx.nim.

Fix some warnings at rlpx.nim.
Commented debug echo in rlpx.nim.
@cheatfate cheatfate requested review from zah and yglukhov June 17, 2018 10:39
@cheatfate
Copy link
Contributor Author

@zah openarray[T] is not supported in closure iterators (it can't be compiled) and also even where it possible to use, its not possible to check if this openarray[T] in stack or not. I wish not to use addr really, but currently its hard to make.

@zah
Copy link
Contributor

zah commented Jun 17, 2018

Let's get to the bottom of the openarray problem. You seem to be saying two things:

  1. readExactly is an async proc, so it cannot be passed an openarray because the internal closure iterator that must be created won't compile. As far as I can tell, this issue can be worked-around by creating a wrapper template for readExactly that accepts a var openarray and calls the existing private lower-level implementation with a pointer/len pair.

  2. openarray is unsafe in general in async code. I remember your analysis on the subject, but I believe openarrays can be fully supported if we enforce the structural handling of the async code, as per my proposal here.

@zah
Copy link
Contributor

zah commented Jun 17, 2018

Btw, Nim can trivially expose a helper allowing one to check if something is on the stack or not. This will be a simple address range check, because Nim needs to store the boundaries of the stack anyway for the purpose of scanning it for GC roots.

@yglukhov
Copy link
Contributor

Imo async function just can't take an openarray in general because the function execution can outlive the passed value no matter if it's stack or heap backed. Thus using openarrays in async proc signatures directly is not an option as i'm seeing it, because an async proc can always be called not only with await, and such calls can not be guaranteed to be safe. What we can do however, is teach async and await about openarrays and make them behave slightly differently. Demo:

proc foo(a: openarray[int]) {.async.} = ...
# Will be actually translated to
proc foo(a: ptr int, eLen: int): Future[void] = ...

And all the await stmts should be transformed accordingly. So that in the end it looks like we're using safe openarrays in async code, but we must fall back to ugly addr and friends when calling those procs from sync code.

@zah
Copy link
Contributor

zah commented Jun 17, 2018

@yglukhov, what do you think about my proposal for enforcing "structural control flow" in the async procs?

status-im/nim-chronos#2

If you haven't already, please also read the article linked in the abstract which is explaining the rationale in a much better detail. I've also expanded the proposal with additional support for spawning async tasks from parallel blocks, so @cheatfate please take a look at that part too.

If we adopt this "structural" model, then openarray is always safe. You are right that the openarray lowering magic will have to happen in the async pragma, not into the closure iterators (because they are created to support non-structural control flow on purpose).

@cheatfate
Copy link
Contributor Author

cheatfate commented Jun 17, 2018

@zah structural control flow do not solve this problem, article you mentioned just trying to move out from goto, to some kind of go, but there still will be pause/continue cycle (when you exit from procedure and return back again). During this cycles stack frames are changing, and that's why openarray[T] can't be used.

@cheatfate
Copy link
Contributor Author

But if compiler allow us to check if openarray[T] is from stack, then we can make a local copy in closure environment (in heap).

@zah
Copy link
Contributor

zah commented Jun 17, 2018

My proposal does solve the problem. Show me an example that you think doesn't work and I'll explain in detail why it works (under enforced awaits).

My impression is that you still haven't read the article, because you keep making wrong characterisations of it. It doesn't "try to move from goto to go". It compares the non-structural approaches to asynchronicity to goto, draws convincing parallels about how introducing structure enabled additional safety features and performance improvements in sync code and shows how the same is true for async.

@yglukhov
Copy link
Contributor

@zah, well enforcing structural control flow might work, in the same way as my suggestion about making async/await transformations more powerful so that they would allow using openarrays, converting them to pointers under the hood. Is that what you mean? If yes, we can't discuss it further in this thread, as currently we're bound by async transformation limitations, and they have to addressed first.

@zah
Copy link
Contributor

zah commented Jun 17, 2018

I think my RFC is the right place to discuss this further, we shouldn't hold this pull request as a hostage :) It looks good to me for merging.

But it does highlight the benefits of the ideas in the RFC and I hope @cheatfate will finally see them as well. Without enforcing the structural control flow, the proposed pointer/len treatment of openarrays won't be safe.

@cheatfate
Copy link
Contributor Author

cheatfate commented Jun 17, 2018

@zah, i have read your article and also i have watched source code of trio, and i'm understanding what i'm saying, but looks like i'm using terms which confuse you. I think you need to investigate asynchronous world more, so we can talk on one language.

There is bunch of callbacks used in asyncdispatch:

  1. Event callbacks.
  2. Future[T] callbacks.
  3. callSoon() callbacks.

Trio uses much less types of callbacks, but it still uses it to transfer control flow. You can't put kqueue/epoll/iocp to every asynchronous procedure to avoid callbacks. Also you can't remove callSoon callbacks. All other its just syntax sugar.

@zah
Copy link
Contributor

zah commented Jun 18, 2018

@cheatfate, there are two separate topics that we have discussed so far:

  1. What is the internal design of the future objects and how are the OS events propagated to the high-level user code. Here, the key difference is that Rust's tokio uses "pull" architecture [1], while our system uses "push" (a chain of callbacks being fired in turn). While, I'd love for our internal design to be the best possible, I fully grant you the expertise here. I only hope that you've given enough though on these alternative ideas and you don't hold established prejudice ("I know it all") that stops you from considering alternatives. I also hope that your conclusions are not based on the state of what is currently possible in Nim, but rather on what is possible in general (something that I've seen you do in the past).

  2. The second more important aspect for me is what are the semantics in the high-level user code. What I'm trying to explain is that by forcing the user to await all operations, certain optimizations become possible. I know this to be true and I don't understand why you fail to acknowledge it. You can convince me that I'm wrong in one very simple way - show me an example that you think won't work and I'll be quick to admit that I was indeed "missing a lot". Such an example can also be just about some limitations of the new API. I think we both agree that it's a very desirable property for the async procs to support the openarray type, so how am I wrong when I claim that it can be supported?

Footnotes:

[1] I'll clarify the point on Rust, because it may be a bit tricky:
Rust's approach is that it uses monomorphically compiled closures to turn the chain of callbacks into a single proc with fully inlined code that steps through all the stages in turn. Monomorphic means closures that don't use indirect function pointers (generic code in C++ that uses functor objects would be an example of this). Such closures can be added to Nim or they can be emulated in the same way code using functors worked in the older versions of C++ (with objects having a poll/exec operation).


Again, right now I really care about the high-level user interface and my RFC is only about that. The internals are a relatively orthogonal topic as far as I can tell (the internals can affect the semantics of the high-level interface, but I think we all agree that async should be done with resumable functions in Nim).

@zah zah merged commit 6bd09b1 into master Jun 18, 2018
@cheatfate cheatfate deleted the asyncdispatch2 branch June 18, 2018 10:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants