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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Flowvar compatible with Async #11724

Open
wants to merge 2 commits into
base: devel
from

Conversation

@rayman22201
Copy link
Contributor

commented Jul 13, 2019

Added some docs as well 馃槃
May still need some cleanup, and it needs tests, but it basically works, as seen on the stream 馃憤

@dom96
dom96 approved these changes Jul 13, 2019
Copy link
Member

left a comment

Beautiful! This opens up thousands of opportunities.

For those that missed the live stream: https://www.twitch.tv/videos/451888943

@@ -206,14 +239,16 @@ proc finished(fv: FlowVarBase) =
# the worker thread waits for "data" to be set to nil before shutting down
owner.data = nil

proc fvFinalizer[T](fv: FlowVar[T]) = finished(fv)
proc fvFinalizer[T](fv: FlowVar[T]) = finito(fv)

This comment has been minimized.

Copy link
@dom96

dom96 Jul 13, 2019

Member

image


proc read*[T](future: FlowVar[T]): T =
## Implement just enough of the Future api to make FlowVar compatible with the async macro.
## Returns the value of the future.

This comment has been minimized.

Copy link
@dom96

dom96 Jul 13, 2019

Member

This is an implementation detail. The docs should focus on what this proc is, not how it was implemented. You can add a note about it at the bottom, but it shouldn't be the highlight.

## Example:
##
## .. code-block::nim
## proc my_thread_task(s: string) : string =

This comment has been minimized.

Copy link
@dom96

dom96 Jul 13, 2019

Member

Nitpick: camelCase (+ below as well)

## return "hello" + s
##
## let a = spawn my_thread_task("world")
## sync()

This comment has been minimized.

Copy link
@dom96

dom96 Jul 13, 2019

Member

This is unnecessary AFAIK, you just need to use the ^ operator.

ready, usesSemaphore, awaited: bool
cv: Semaphore # for 'blockUntilAny' support
ai: ptr AwaitInfo
idx: int
data: pointer # we incRef and unref it to keep it alive; note this MUST NOT
# be RootRef here otherwise the wrong GC keeps track of it!
owner: pointer # ptr Worker
event*: AsyncEvent # makes Flowvar work with async

This comment has been minimized.

Copy link
@dom96

dom96 Jul 13, 2019

Member

I think this can be private :)

@@ -224,6 +259,7 @@ proc nimFlowVarSignal(fv: FlowVarBase) {.compilerProc.} =
signal(fv.ai.cv.c)
if fv.usesSemaphore:
signal(fv.cv)
fv.event.trigger()

proc awaitAndThen*[T](fv: FlowVar[T]; action: proc (x: T) {.closure.}) =

This comment has been minimized.

Copy link
@dom96

dom96 Jul 13, 2019

Member

We need to deprecate these. If you have some time please create a seperate PR to do that :)

proc complete*(future: FutureBase) =
## Completes a base ``future``.
#assert(not future.finished, "Future already finished, cannot finish twice.")
#checkFinished(future)

This comment has been minimized.

Copy link
@dom96

dom96 Jul 13, 2019

Member

We need this, at the very least the above assert will do.

@@ -196,6 +196,15 @@ proc add(callbacks: var CallbackList, function: CallbackFunc) =
last = last.next
last.next = newCallback

proc complete*(future: FutureBase) =
## Completes a base ``future``.

This comment has been minimized.

Copy link
@dom96

dom96 Jul 13, 2019

Member

Emphasise in the docs that this likely shouldn't be used unless you really know what you're doing :)

@zevv

This comment has been minimized.

Copy link

commented on 7fac17e Jul 14, 2019

(Jotting things down here, IRC stuff might get lost in the noise)

I got kind of stuck with the ioselectors event refactoring. The fd of the event pipe is used as the index to get to the user data, so sharing the same fd will mess things up, nothing the ioselector demuxer can do about it.

I think this will need work at the async layer as well:

  • ioselectors need an extra mechanism to pass data through a trigger() - this should be fairly trivial, the actual data going in the write() can be passed as an optional argument. The received data can be added to the ReadyKey object.
  • async should create only one SelectEvent which is then used for all flowvars, and use the new trigger event data to do the bookkeeping.

Does that make sense?

This comment has been minimized.

Copy link

replied Jul 16, 2019

@rayman22201 Friendly ping

@mratsim mratsim added this to the v1 milestone Jul 30, 2019

@narimiran

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Any update on this one?

@rayman22201

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@narimiran We got a bit demotivated because of the Chronos vs. Stdlib async drama.
This PR is moot because two things need to happen: (Not in any particular order)

  1. the stdlib threadpool needs to be replaced with a better implementation (probably @yglukhov's https://github.com/yglukhov/threadpools)

  2. The stdlib ioselectors needs to be fixed so that it does not eat up all of the socket fd's on your system. As described by @zevv in his comment above: #11724 (comment)

@dom96

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

@rayman22201 is #1 necessary to get this working too?

@rayman22201

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@dom96. Not at all. I didn't mean to imply that they have to be done in some order. Both tasks are independent.

Doing #1 will probably require a rework of this PR though. I personally would rather close this PR, do #1 first, then redo this PR based on Yuri's threadpool.

@Araq Araq modified the milestones: v1, v1.1 Aug 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can鈥檛 perform that action at this time.