Skip to content

bufferByCountAndTime, RequireQualifiedAccess#22

Closed
eulerfx wants to merge 6 commits intofsprojects:masterfrom
eulerfx:master
Closed

bufferByCountAndTime, RequireQualifiedAccess#22
eulerfx wants to merge 6 commits intofsprojects:masterfrom
eulerfx:master

Conversation

@eulerfx
Copy link
Copy Markdown
Contributor

@eulerfx eulerfx commented May 7, 2015

Also addresses #20

Wondering whether the currently commented operation is a good abstraction:

val bufferBy : bound:Async<unit> -> s:AsyncSeq<'T> -> AsyncSeq<'T []>

This is used to implement bufferByTime and is meant to be an analog to Observable.Buffer, however it can't be used to implement bufferByCount for example because doesn't have access to the buffer itself. Maybe a more general operation is in order?

@eulerfx
Copy link
Copy Markdown
Contributor Author

eulerfx commented May 8, 2015

@forki would it be possible to grant me write access to this repo?

@eulerfx eulerfx changed the title bufferByCountAndTime bufferByCountAndTime, RequireQualifiedAccess May 10, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does allowing a timeout of 0 (= wait indefinitely) make sense?

@dsyme
Copy link
Copy Markdown
Contributor

dsyme commented May 13, 2015

@eulerfx I've spoken with @tpetricek about this. We need to address #15 before we go too much further, since the current abstraction is pretty much flawed w.r.t. try/finally and try/catch. We'll be working on that. Realistically this means moving to an IAsyncEnumerator/IAsyncEnumerable model.

@eulerfx
Copy link
Copy Markdown
Contributor Author

eulerfx commented May 13, 2015

@dsyme alright sounds good. I'll have some time weekend after next and I can do a lot of the gruntwork to re-implement all operations based on IAsyncEnumerable, starting with what you posted.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I'm sceptical about the way Async.chooseBoth works in the existing implementation. It looks likely to create an ever-lengthening chain of tasks as you repeatedly choose between the signal and a move. The signal is only started once, but each time the "move" wins then a delegating task/async is returned. Doesn't feel right, we need to add stress tests for this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I'll take a look. I did a stress test comparing two different implementations of AsyncSeq.merge - one using a BlockingCollection to hold incoming values from two sequences and one using Async.chooseBoth and the latter was marginally faster, better GC footprint, etc. Overall, they were generally comparable. I'll do a stress test focusing on Async.chooseBoth. One change that I think makes sense is to not use Async.StartWithContinuations since that uses the current thread and may be overly unfair as a result. Using Async.Start with explicit callbacks instead.

@dsyme
Copy link
Copy Markdown
Contributor

dsyme commented May 14, 2015

Hi @eulerfx - we can start with #23 I think, but I'm certain the implementations can be cleaner and more performant. If you can make improvements they would be very much appreciated.

@dsyme
Copy link
Copy Markdown
Contributor

dsyme commented May 29, 2015

@eulerfx - Could you update this for #23? Thanks!

@eulerfx
Copy link
Copy Markdown
Contributor Author

eulerfx commented May 31, 2015

#26

@eulerfx eulerfx closed this Jun 1, 2015
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.

2 participants