AsyncSeq.cache fails with SynchronizationLockException about fsharp.control.asyncseq HOT 9 CLOSED

I played with this a little. The SynchronizationLockException could be fixed easily by changing the Monitor to ManualResetEventSlim or SemaphoreSlim(1,1). Now the monitor is exited from a different thread and such exception is documented on theMonitorMSDN page. The build-in test forcache` passes with the changes and the exception goes away in the example, but the example then hangs in a endless loop. Couldn't fix that last night.

from fsharp.control.asyncseq.

Thanks, created a failing test for this:

[<Test>]
let ``Async.cache should work``() =  
  let expected = List.init 1000 id
  let effects = ref 0
  let s = asyncSeq {
    for item in expected do      
      yield item
      do! Async.Sleep 1
      incr effects
  }
  let cached = s |> AsyncSeq.cache
  let actual = cached |> AsyncSeq.toList
  let actual = cached |> AsyncSeq.toList
  Assert.True((expected = actual), "cached sequence was different from the original")
  Assert.True((!effects = expected.Length))

It fails when the Async.Sleep is interleaved.

One more thing to consider is the waiting strategy. While ManualResetEventSlim works, just like Monitor it involves blocking a thread during wait. ManualResetEventSlim has a lazy reference to a WaitHandle which can be used to wait in a non-blocking way, but referencing that defeats the purpose of the ManualResetEventSlim over simply using ManualResetEvent. Ideally, we would have an async based ManualResetEventSlim which spins before resorting to the WaitHandle after which it waits in a non-blocking way with Async.AwaitWaitHandle.

Two options at this point are 1) ManualResetEventSlim with the blocking Wait which spins, this does show clear performance improvements over the non-blocking wait using WaitHandle in many scenarios and 2) ManualResetEvent with the non-blocking wait, which can make more efficient use of threads, but is less performant in the case when the wait time is short.

from fsharp.control.asyncseq.

SemaphoreSlim has async wait method, with 1,1 it is effectively MRE
On Sep 4, 2015 7:08 PM, "Lev Gorodinski" [email protected] wrote:

Thanks, created a failing test for this:

[]
let Async.cache should work() =
let expected = List.init 1000 id
let effects = ref 0
let s = asyncSeq {
for item in expected do
yield item
do! Async.Sleep 1
incr effects
}
let cached = s |> AsyncSeq.cache
let actual = cached |> AsyncSeq.toList
let actual = cached |> AsyncSeq.toList
Assert.True((expected = actual), "cached sequence was different from the original")
Assert.True((!effects = expected.Length))

It fails when the Async.Sleep is used after yield.

One more thing to consider is the waiting strategy. While
ManualResetEventSlim works, just like Monitor it involves blocking a
thread during wait. ManualResetEventSlim has a lazy reference to a
WaitHandle which can be used to wait in a non-blocking way, but
referencing that defeats the purpose of the ManualResetEventSlim over
simply using ManualResetEvent. Ideally, we would have an async based
ManualResetEventSlim which spins before resorting to the WaitHandle after
which it waits in a non-blocking way with Async.AwaitWaitHandle.

Two options at this point are 1) ManualResetEventSlim with the blocking
Wait which spins, this does show clear performance improvements over the
non-blocking wait using WaitHandle in many scenarios and 2)
ManualResetEvent with the non-blocking wait, which can make more
efficient use of threads, but is less performant in the case when the wait
time is short.

—
Reply to this email directly or view it on GitHub
#33 (comment)
.

from fsharp.control.asyncseq.

Async.Await... any non F# things is usually slow. We could get TaskAwaiter
from Semaphore.WaitAsync and move the current Try/Finally logic inside
OnCompletion action. With this, we will also avoid the expensive
Try/Finally of AsyncSeq altogether. Writing from mobile, could not post
code now, but I used this pattern recently and performance is the fastest
one could get from any async machinery in .NET
On Sep 4, 2015 7:25 PM, "Victor Baybekov" [email protected] wrote:

SemaphoreSlim has async wait method, with 1,1 it is effectively MRE
On Sep 4, 2015 7:08 PM, "Lev Gorodinski" [email protected] wrote:

Thanks, created a failing test for this:

[]
let Async.cache should work() =
let expected = List.init 1000 id
let effects = ref 0
let s = asyncSeq {
for item in expected do
yield item
do! Async.Sleep 1
incr effects
}
let cached = s |> AsyncSeq.cache
let actual = cached |> AsyncSeq.toList
let actual = cached |> AsyncSeq.toList
Assert.True((expected = actual), "cached sequence was different from the original")
Assert.True((!effects = expected.Length))

It fails when the Async.Sleep is used after yield.

One more thing to consider is the waiting strategy. While
ManualResetEventSlim works, just like Monitor it involves blocking a
thread during wait. ManualResetEventSlim has a lazy reference to a
WaitHandle which can be used to wait in a non-blocking way, but
referencing that defeats the purpose of the ManualResetEventSlim over
simply using ManualResetEvent. Ideally, we would have an async based
ManualResetEventSlim which spins before resorting to the WaitHandle
after which it waits in a non-blocking way with Async.AwaitWaitHandle.

Two options at this point are 1) ManualResetEventSlim with the blocking
Wait which spins, this does show clear performance improvements over the
non-blocking wait using WaitHandle in many scenarios and 2)
ManualResetEvent with the non-blocking wait, which can make more
efficient use of threads, but is less performant in the case when the wait
time is short.

—
Reply to this email directly or view it on GitHub
#33 (comment)
.

from fsharp.control.asyncseq.

I've seen Semaphore.WaitAsync have similar overhead to just waiting via WaitHandle. In some cases, simply doing a Semaphore.Wait is faster.

Not quite sure what you mean by getting rid of the try/finally - do you have an example of what you have in mind?

from fsharp.control.asyncseq.

Will post later
On Sep 4, 2015 7:50 PM, "Lev Gorodinski" [email protected] wrote:

I've seen Semaphore.WaitAsync have similar overhead to just waiting via
WaitHandle. In some cases, simply doing a Semaphore.Wait is faster.

Not quite sure what you mean by getting rid of the try/finally - do you
have an example of what you have in mind?

—
Reply to this email directly or view it on GitHub
#33 (comment)
.

from fsharp.control.asyncseq.

The idea was to use this pattern:

              let task = semaphore.WaitAsync()
              let awaiter = task.GetAwaiter()
              awaiter.OnCompleted(fun _ ->
                if i >= cache.Count then 
                    let! move = ie.MoveNext()
                    cache.Add(move)
                    iRef := i + 1
                Diagnostics.Debug.Assert(semaphore.CurrentCount = 0)
                semaphore.Release() |> ignore
              )

Here try/finally becomes an F# language construct instead of AsynSeq builder method. But it doesn't work now because let! move = ie.MoveNext() cannot be called inside OnCompleted. Even if it worked, AsynSeq is built around async and we would have to cross the TPL/async boundary later. Recently I worked with an API that is build on TPL Tasks (basically it is TaskSeq, an async sequence on top of Tasks), but the project was in F#. I noticed a huge performance impact of Async.AwaitTask and Async.StartAsTask compared to TPL, and my solution was to remove Async completely (sample). Here it won't work, and I hope that Async.AwaitWaitHandle doesn't add unnecessary overhead since WaitHandles are basic primitives and a part of System.Threading, not TPL.

from fsharp.control.asyncseq.

@dsyme I think this issue can be closed re PR #34, however might be worth having a separate discussion wrt waiting as noted: #33 (comment)

This is more related to Async than AsyncSeq though.

from fsharp.control.asyncseq.

OK, yes, let's close this and add a new issue re perf if needed

from fsharp.control.asyncseq.