Refactor Binds to Source members #12

TheAngryByrd · 2022-11-30T00:18:04Z

Proposed Changes

This uses the Source member overloads to provide a more stripped down version of the ColdTask and CancellableTask CEs.

Similar to the work I did in demystifyfp/FsToolkit.ErrorHandling#83

Types of changes

What types of changes does your code introduce to IcedTasks?
Put an x in the boxes that apply

Refactoring

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

Build and tests pass locally
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have added necessary documentation (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

TheAngryByrd · 2022-11-30T00:21:26Z

@dsyme do you know of any reason I should not use Source member overloads like this to reduce the complexity of a resumable CE?

cc @abelbraaksma @NinoFloris

abelbraaksma · 2022-11-30T17:09:38Z

Let's /cc @gusty. He mentioned to me the other day that this has been around since a long time, but only covered in the spec, not in docs. As far as I know this is safe to do, but maybe Gus can glance over these changes if he knows the intrinsics of this approach?

NinoFloris · 2022-11-30T17:14:15Z

Not as far as I know @TheAngryByrd. There are of course some overload resolution cases where having the continuation arguments in the same resolution as the value can influence the outcome to be a better fit but in almost all cases I've been able to restructure to still be able to use Source.

abelbraaksma

This is a pretty awesome change. Are the timing differences due to .NET 7 or caused by the move to Source? They are quite impressive.

abelbraaksma · 2022-11-30T17:11:24Z

benchmarks/FSharpBenchmarks/AsyncBenchmarks.fs

@@ -324,6 +324,8 @@ module Helpers =
 [<CategoriesColumn>]
 type AsyncBenchmarks() =

+    let getTempFileName () =
+        Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("n"))


Just a thought: if you dump these in an IDisposable, you can actually delete the files after the tests. Or use setup/teardown, which I believe BenchmarkDotnet also supports.

Yeah, i need to steal my disposabledirectory/file code from other repos

abelbraaksma · 2022-11-30T17:13:13Z

...arks/BenchmarkDotNet.Artifacts/results/IcedTasks.Benchmarks.AsyncBenchmarks-report-github.md

-|  AsyncBinds_cancellableTask_bindCancellableTask |    AsyncBinds |  20,201.2 μs |    400.21 μs |    852.88 μs |  20,049.0 μs |  1.25 |    0.15 |    31.2500 |       - |      - |     320 KB |
+|                                          Method |    Categories |         Mean |        Error |       StdDev |       Median |  Ratio | RatioSD |       Gen0 |    Allocated | Alloc Ratio |
+|------------------------------------------------ |-------------- |-------------:|-------------:|-------------:|-------------:|-------:|--------:|-----------:|-------------:|------------:|
+|                          AsyncBinds_CSharpTasks |    AsyncBinds |   3,329.6 μs |     28.27 μs |     26.45 μs |   3,334.1 μs |   1.00 |    0.00 |     3.9063 |    109.39 KB |        1.00 |


This (AsyncBinds_CSharpTasks /AsyncBinds) went from 17μs to 3μs? Is this due to the changed setup or can "old vs new" be compared? It's a pretty amazing perf gain!

I wouldn't read too much into these as this has changed machines.

abelbraaksma · 2022-11-30T17:16:24Z

src/IcedTasks/CancellableTask.fs

                and ^Awaiter: (member get_IsCompleted: unit -> bool)
                and ^Awaiter: (member GetResult: unit -> 'TResult1)>
                (
                    sm: byref<ResumableStateMachine<CancellableTaskStateMachineData<'TOverall>>>,
-                    task: ^TaskLike,
+                    [<InlineIfLambda>] getAwaiter: CancellationToken -> ^Awaiter,


Not sure this makes a difference, since the argument is already static (i.e., it uses an SRTP constraint, so it has to be inlined). Did you see a difference in the generated code?

No I still haven't gotten around to investigating the different state machine generation and yeah I'm unsure how often I'd actually need InlineIfLambda if at all in most cases in this file anyway.

For (some) insights, see this answer by Don: dotnet/fsharp#14123 (comment)

gusty · 2022-11-30T17:22:33Z

I don't think there's something special about Source it can be overloaded as any other methods at the risk of the scaling (logical) limitations of doing Ad-hoc overloading for the sole purpose of saving keystrokes.

abelbraaksma · 2022-11-30T23:02:13Z

@gusty I think the main use-case here is that by using Source, you'd have less boilerplate to write in the Bind and Yield functions. In other words, it's not just saving keystrokes, but making the implementation more resilient, where Source is used like a "gateway" and the rest is unified. At least that's what I take from it.

The main question at hand is: is there any caveat to using it? (also asking because of my own interest in the method).

gusty · 2022-11-30T23:36:46Z

you'd have less boilerplate to write in the Bind and Yield functions. In other words, it's not just saving keystrokes,

Well, you're saving keystrokes there already. Ideally the user should explicitly convert. Moving it to Source won't change anything in that regard.

But going to your main question:

The main question at hand is: is there any caveat to using it? (also asking because of my own interest in the method).

Not that I know. I never used it in my OSS projects, but I used it in many prototypes and I wouldn't hesitate to use it in any OSS project, it's just that I still didn't find a problem that it would solve there.

abelbraaksma · 2022-11-30T23:47:53Z

Great, thanks! Looking at the diff here, the main use case is simplification. That, of course, isn't needed in each and every CE.

TheAngryByrd · 2022-12-01T00:32:14Z

Thank you everyone for your feedback. Highly appreciated!

@TheAngryByrd

## [0.3.2-beta001] - 2022-11-30 ### Changed - [Refactor Binds to Source members](#12) Credits @TheAngryByrd

@TheAngryByrd

## [0.3.2-beta002] - 2022-11-30 ### Changed - [Refactor Binds to Source members](#12) Credits @TheAngryByrd - [Expand TFMs to netstandard2.0 and 2.1](#13) Credits @TheAngryByrd - [InlineIfLambda was just adding compile time without benefit](#14) Credits @TheAngryByrd - [Refactor Binds to Source members](#12) Credits @TheAngryByrd

@TheAngryByrd

## [0.3.2] - 2022-11-30 ### Changed - [Refactor Binds to Source members](#12) Credits @TheAngryByrd - [Expand TFMs to netstandard2.0 and 2.1](#13) Credits @TheAngryByrd - [InlineIfLambda was just adding compile time without benefit](#14) Credits @TheAngryByrd

TheAngryByrd added 3 commits November 28, 2022 20:36

Using Source members for ColdTasks

69f40c7

CancellationTask using Source overloads

3ef52c9

Strips to minimum bind/returnfrom needed

0741a4f

abelbraaksma reviewed Nov 30, 2022

View reviewed changes

TheAngryByrd merged commit 88d5c6a into master Dec 1, 2022

TheAngryByrd changed the title ~~Source members~~ Refactor Binds to Source members Dec 1, 2022

TheAngryByrd added a commit that referenced this pull request Dec 1, 2022

Bump version to 0.3.2-beta001

11bd02b

## [0.3.2-beta001] - 2022-11-30 ### Changed - [Refactor Binds to Source members](#12) Credits @TheAngryByrd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Binds to Source members #12

Refactor Binds to Source members #12

TheAngryByrd commented Nov 30, 2022 •

edited

Loading

TheAngryByrd commented Nov 30, 2022

abelbraaksma commented Nov 30, 2022

NinoFloris commented Nov 30, 2022

abelbraaksma left a comment

abelbraaksma Nov 30, 2022

TheAngryByrd Nov 30, 2022

abelbraaksma Nov 30, 2022 •

edited

Loading

TheAngryByrd Nov 30, 2022

abelbraaksma Nov 30, 2022

TheAngryByrd Nov 30, 2022

abelbraaksma Nov 30, 2022

gusty commented Nov 30, 2022

abelbraaksma commented Nov 30, 2022 •

edited

Loading

gusty commented Nov 30, 2022

abelbraaksma commented Nov 30, 2022

TheAngryByrd commented Dec 1, 2022

Refactor Binds to Source members #12

Refactor Binds to Source members #12

Conversation

TheAngryByrd commented Nov 30, 2022 • edited Loading

Proposed Changes

Types of changes

Checklist

Further comments

TheAngryByrd commented Nov 30, 2022

abelbraaksma commented Nov 30, 2022

NinoFloris commented Nov 30, 2022

abelbraaksma left a comment

Choose a reason for hiding this comment

abelbraaksma Nov 30, 2022

Choose a reason for hiding this comment

TheAngryByrd Nov 30, 2022

Choose a reason for hiding this comment

abelbraaksma Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

TheAngryByrd Nov 30, 2022

Choose a reason for hiding this comment

abelbraaksma Nov 30, 2022

Choose a reason for hiding this comment

TheAngryByrd Nov 30, 2022

Choose a reason for hiding this comment

abelbraaksma Nov 30, 2022

Choose a reason for hiding this comment

gusty commented Nov 30, 2022

abelbraaksma commented Nov 30, 2022 • edited Loading

gusty commented Nov 30, 2022

abelbraaksma commented Nov 30, 2022

TheAngryByrd commented Dec 1, 2022

TheAngryByrd commented Nov 30, 2022 •

edited

Loading

abelbraaksma Nov 30, 2022 •

edited

Loading

abelbraaksma commented Nov 30, 2022 •

edited

Loading