New async core #4309

cheatfate · 2016-06-10T20:48:42Z

ioselectors.nim features:

there no memory alloc/dealloc operations anymore, there only one in newSelector.
support for timers (EVFILT_TIMER on BSD, timerfd on Linux, RegisterWaitForSingleObject on Windows IOCP)
support for process monitoring (EVFILT_PROC on BSD, signalfd on Linux, RegisterWaitForSingleObject on Windows IOCP)
support for signal monitoring (EVFILT_SIGNAL on BSD, signalfd on Linux, Windows is not supported here because of lack of signals)
support for application-driven events (EVFILT_USER on BSD, eventfd on Linux, RegisterWaitForSingleObject on Windows IOCP)
compiles with "--threads:on" and threadsafe in mind, with assumption that multiple threads will not work with one particular socket.

asyncdispatch.nim features:

Callback functions:

addTimer() - watch for hardware timer timeout and schedule callback.
addSignal() - watch for Posix signal and schedule callback.
addProcess() - watch for process exit and schedule callback.
addEvent() - watch for application-driven event triggers and schedule callback.

Futures:

asyncSleep() - same as sleepAsync() but uses hardware timers.
asyncProcess() - returns Future (which completes only when process exited)
asyncSignal() - returns Future (which completes only when Posix signal is fired)
asyncEvent() - returns Future (which completes only when event is triggered)

Fully supported OS: Windows (IOCP), FreeBSD, OpenBSD, NetBSD, MacOSX, Linux.
Partially supported OS: Solaris, Windows (select)
When using Solaris or Windows only sockets and application-driven events are supported.

Changelog since v1 of this PR:

There some changes made to ioselectors.nim, so now it uses generics as application-driven data. This patch made possible to avoid some memory allocations in asyncdispatch.nim code.

TODO: Adopt Solaris eventports and maybe /dev/poll in future.

cheatfate · 2016-06-11T08:36:48Z

I have forgot, i also added isEmpty() template for ioselectors.nim, so bug #4262 is resolved in this PR.

… in `lib/pure`.

dom96 · 2016-06-11T18:56:07Z

lib/pure/asyncdispatch.nim

@@ -1680,4 +1680,4 @@ proc waitFor*[T](fut: Future[T]): T =
  while not fut.finished:
    poll()

-  fut.read
+  fut.read


@cheatfate what text editor are you using? Whatever it is, it's not adding a newline at the end of files which is incorrect.

I'm using SublimeText 3.

dom96 · 2016-06-11T19:18:51Z

I know that @Araq asked you to put ioselectors and your modified asyncdispatch into a new folder, but that makes it difficult to review. I am also still not fine with giving up a 415 LOC selectors in favour of a 2500 LOC ioselectors in the future.

I would prefer if the features you implemented in ioselectors were ported to selectors slowly. You should start by creating a PR that removes excessive allocations in the selectors module, that way we can safely review it and then we can think about introducing more features into it.

dom96 · 2016-06-11T19:21:30Z

Another thing, asyncdispatch follows a certain naming scheme which your PR breaks. Any procedure beginning with async shouldn't return a Future, only procedures that end with Async should. In the current asyncdispatch, this is currently followed. For example, asyncCheck accepts a Future as a parameter whereas sleepAsync returns a Future.

…t can returns ERROR_IO_PENDING.

cheatfate · 2016-06-11T21:29:32Z

@dom96 about certain naming scheme, is there any place, where i can read about this naming schemes?

Araq · 2016-06-11T22:39:56Z

I would prefer if the features you implemented in ioselectors were ported to selectors slowly. You should start by creating a PR that removes excessive allocations in the selectors module, that way we can safely review it and then we can think about introducing more features into it.

That's a good way to do things but that's overly taxing cheatfate. He's been working on this PR for months now and afaik it's really battle-tested and has better performance than the current asyncdispatch. And yet it's compatible with the old interface! That's nothing to dismiss.

More random notes:

We can use any 'diff' tool of your choice to compare the 2 asyncdispatch implementations.
We can adapt the code to your conventions afterwards.
It doesn't break anything since people need to opt-in to use the improved asyncdispatch. Take as much time as you like to become familiar with the improvements.

dom96 · 2016-06-11T23:06:39Z

@dom96 about certain naming scheme, is there any place, where i can read about this naming schemes?

I'm afraid it's not documented anywhere, but I have designed asyncdispatch with it in mind.

That's a good way to do things but that's overly taxing cheatfate. He's been working on this PR for months now and afaik it's really battle-tested and has better performance than the current asyncdispatch. And yet it's compatible with the old interface!

I can continue to argue against this here (after arguing on IRC), but in the interest of time I will just say: fine, review it and merge it but don't expect me to be happy about it or to agree with the decision.

cheatfate · 2016-06-11T23:15:03Z

@dom96 i dont think names is a big problem, please write here your variants of names and i can change it.
And more on ioselectors.nim, there 500 LOCs of tests, we can easily remove them, also we can remove windows select() code and it think it will be near 1000-1500 LOCs.

dom96 · 2016-06-11T23:40:41Z

asyncSleep() -> sleepAsyncHardware(), or add a flag to sleepAsync, or just use hardware timers by default?
asyncProcess() -> processAsync(), or more appropriate (and consistent with osproc) would likely be: waitForProcessAsync()
asyncSignal() - signalAsync(), likely more appropriate would be waitForSignalAsync
asyncEvent() - eventAsync(), likely more appropriate would be waitForEventAsync

I also think the "callback" functions should get different names.

cheatfate · 2016-06-12T00:02:29Z

@dom96, so in code it will looks like:
waitFor waitForProcessAsync(pid) or await waitForProcessAsync(pid)
waitFor waitForSignalAsync(signal) or await waitForSignalAsync(signal)
waitFor waitForEventAsync(event) or await waitForEventAsync(event)
and as for me is a little bit weird... too many wait in one line.

about sleepAsync(), i think the biggest problem of this function - using epochTime(), because it really slow (while converting timespec to float). And maybe its a little overhead to use hardware timer for simple Sleep.

And whats wrong with callback functions? I think they simply copy your semantic of addRead/addWrite.

dom96 · 2016-06-12T00:27:50Z

@dom96, so in code it will looks like:
waitFor waitForProcessAsync(pid) or await waitForProcessAsync(pid)
waitFor waitForSignalAsync(signal) or await waitForSignalAsync(signal)
waitFor waitForEventAsync(event) or await waitForEventAsync(event)
and as for me is a little bit weird... too many wait in one line.

True, in that case it doesn't look nice. But client code likely won't be using it anyway. For example, you will have a startProcess procedure (defined in a new asyncproc module perhaps) that will use waitForProcessAsync internally. You will then be able to: await startProcess("wget http://nim-lang.org/download/nim.zip").

about sleepAsync(), i think the biggest problem of this function - using epochTime(), because it really slow (while converting timespec to float). And maybe its a little overhead to use hardware timer for simple Sleep.

If you would like to provide two versions of this procedure then that's fine. But give the "hardware timer" version a more descriptive name please.

And whats wrong with callback functions? I think they simply copy your semantic of addRead/addWrite.

Yes, but addRead/addWrite are defined in the selectors module not in asyncdispatch. Whereas your callback functions are defined in asyncdispatch aren't they?

cheatfate · 2016-06-12T11:43:28Z

@dom96, please check asyncdispatch.nim code, and maybe selectors.nim code.

Yes, but addRead/addWrite are defined in the selectors module not in asyncdispatch. Whereas your callback functions are defined in asyncdispatch aren't they?

Just because
asyncdispatch.addRead()
asyncdispatch.addWrite()

dom96 · 2016-06-12T12:22:16Z

Ahh. Forgot those were in there. In that case your callback functions are ok.

cheatfate · 2016-06-12T12:32:36Z

@Araq, please help us with names, just because asyncdispatch.nim is a public api, so names must be nice, i think.

Araq · 2016-06-12T13:49:34Z

IMO:

asyncSleep() -> sleepAsyncHardware()
asyncProcess() -> processAsync()
asyncSignal() - signalAsync()
asyncEvent() - eventAsync()

cheatfate · 2016-06-12T13:56:18Z

what about timerAsync() for sleepAsyncHardware()?

karantin2020 · 2016-06-12T18:01:28Z

Hello. Could you add more documentation please? Need more info about new procs usage. May be more examples.
Do you have plans when PR will be accepted? Changes are very good.

andreaferretti · 2016-06-21T08:19:51Z

IMHO, it would be better to merge the whole PR at once. Being author of a library that depends on async, I would prefer to fix everything at once rather than having to perform a few changes every few days as the features in ioselectors are ported to selectors slowly

Araq · 2016-06-22T20:01:29Z

lib/pure/ioselectors.nim

+      ## An object which holds user defined event
+
+  const
+    EVENT_READ*       = 0x00000001 ## Descriptor is available for read


Unless this is somehow directly mapped to the OS specific constants, this should be an enum and the combination of flags a set[myenum].

I have made benchmarks for simple boolean arithmetic and usage of sets. Simple boolean arithmetic is twice as fast than sets.

I doubt these benchmarks. What exactly did you benchmark?

Araq · 2016-06-23T07:24:17Z

lib/pure/ioselectors.nim

+          part: array[16, T]
+        SharedArray {.unchecked.}[T] = array[0..100_000_000, T]
+
+      proc newSharedArray[T](nsize: int): ptr SharedArray[T] =


Name it allocSharedArray.

cheatfate added 3 commits June 10, 2016 23:42

New async core with generics

e29d1da

Forgot to remove some debugging echos from asyncdispatch.nim

e390dca

Removed one more part of debugging code.

495a5b5

Moved asyncdispatch.nim to lib/incoming, restored old asyncdispatch…

b27b0a9

… in `lib/pure`.

dom96 reviewed Jun 11, 2016
View reviewed changes

cheatfate added 2 commits June 12, 2016 00:15

add stackFrame pragma to Windows callback handle code.

9a2ee05

Removing checks for errorCode of unregisterWait() function, because i…

9d94dd7

…t can returns ERROR_IO_PENDING.

UnregisterWait must be first on cleanup

73ae6c9

Araq reviewed Jun 22, 2016
View reviewed changes

Araq reviewed Jun 23, 2016
View reviewed changes

dom96 mentioned this pull request Jun 25, 2016

New async core part 1 #4402

Merged

cheatfate closed this Jul 11, 2016

cheatfate deleted the ioasyncv2 branch October 21, 2016 13:27

cheatfate restored the ioasyncv2 branch January 31, 2017 15:30

cheatfate deleted the ioasyncv2 branch January 31, 2017 15:37

cheatfate restored the ioasyncv2 branch January 31, 2017 15:40

cheatfate deleted the ioasyncv2 branch January 31, 2017 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New async core #4309

New async core #4309

cheatfate commented Jun 10, 2016

cheatfate commented Jun 11, 2016

dom96 Jun 11, 2016

cheatfate Jun 11, 2016

dom96 commented Jun 11, 2016

dom96 commented Jun 11, 2016

cheatfate commented Jun 11, 2016

Araq commented Jun 11, 2016

dom96 commented Jun 11, 2016

cheatfate commented Jun 11, 2016

dom96 commented Jun 11, 2016

cheatfate commented Jun 12, 2016

dom96 commented Jun 12, 2016

cheatfate commented Jun 12, 2016

dom96 commented Jun 12, 2016

cheatfate commented Jun 12, 2016

Araq commented Jun 12, 2016

cheatfate commented Jun 12, 2016

karantin2020 commented Jun 12, 2016

andreaferretti commented Jun 21, 2016

Araq Jun 22, 2016

cheatfate Jun 22, 2016

Araq Jun 23, 2016

Araq Jun 23, 2016

New async core #4309

New async core #4309

Conversation

cheatfate commented Jun 10, 2016

cheatfate commented Jun 11, 2016

dom96 Jun 11, 2016

Choose a reason for hiding this comment

cheatfate Jun 11, 2016

Choose a reason for hiding this comment

dom96 commented Jun 11, 2016

dom96 commented Jun 11, 2016

cheatfate commented Jun 11, 2016

Araq commented Jun 11, 2016

dom96 commented Jun 11, 2016

cheatfate commented Jun 11, 2016

dom96 commented Jun 11, 2016

cheatfate commented Jun 12, 2016

dom96 commented Jun 12, 2016

cheatfate commented Jun 12, 2016

dom96 commented Jun 12, 2016

cheatfate commented Jun 12, 2016

Araq commented Jun 12, 2016

cheatfate commented Jun 12, 2016

karantin2020 commented Jun 12, 2016

andreaferretti commented Jun 21, 2016

Araq Jun 22, 2016

Choose a reason for hiding this comment

cheatfate Jun 22, 2016

Choose a reason for hiding this comment

Araq Jun 23, 2016

Choose a reason for hiding this comment

Araq Jun 23, 2016

Choose a reason for hiding this comment