[WIP] Seq Composer #1570

manofstick · 2016-09-28T09:48:25Z

A Seq replacement somewhat similar (but better!) performance to System.Linq extension methods. Will attempt to replicate all functionality under the new method, but not required. i.e. compatible with existing implementation.

Seq module functions that produce seqs - will at least have a look at them all to determine if they can be reasonably moved over.

Seq functions that consume seqs (undecided if will provide special implementations or not, as performance is really improved within the chain of the seqs rather than just the tail. But maybe.)

manofstick · 2016-09-30T10:33:05Z

@KevinRansom - I'm having grief trying to the IL output of the error that I'm getting. My machine isn't running all the tests (dying earlier? Why? I don't know, I don't care for this machine very much though...) so I can't see the output of the SeqExpressionSteppingTest1 error that is occurring. Anyway, any assistance (pointing at me to RTFM is acceptable!) would be appreciated.

manofstick · 2016-09-30T10:40:11Z

@liboz

Hey Libo, any chance you want to collaborate on this? It started inspired a bit by #1528

It's not currently building fully due to ILCompare coming up with a mismatch, but I think this is just a technicality (i.e. crankin' the handle). I'll like to get as much of the Seq.* functions over as possible to this framework.

I'm pretty busy, so not sure how much I'll be able to achieve in the near future. First I guess I would just like some eyeballs on it to see it's not completely insane, secondly I'd like to add whatever other functions would head over ("choose" is an obvious next.)

Anyway, let us know.

liboz · 2016-09-30T11:05:35Z

I would be interested and I'll try to look at this more closely today. That said @dsyme's post in the other fusing Seq.map PR seems to indicate that the FSharp team might not be interested in these kinds of changes, at least at this point. So I'm not sure if there will be a lot of momentum/interest in this.

manofstick · 2016-09-30T11:14:35Z

@liboz

My interpretation of @dsyme's response to #1525 was that he wasn't interested in the compiler performing these kinds of optimizations. Now my interpretation could be wrong, so let us know Don!

Anyway, assuming I'm not wrong, and he's still happy for library improvements, this is a pure fsharp.core change. No compilation changes required! I.e. this is very similar to what you proposed, just expanded upon to make it more flexible to all functions, rather than just collapsing map functions.

Have a look and see what you think.

dsyme · 2016-09-30T11:21:07Z

My interpretation of @dsyme's response to #1525 was that he wasn't interested in the compiler performing these kinds of optimizations.

Library versions of these optimizations are definitely worth considering and consistent with what we do already

liboz · 2016-10-01T04:34:32Z

src/fsharp/FSharp.Core/seq.fs

+
+                abstract Composer : SeqComponent<'U,'V> -> SeqComponent<'T,'V>
+
+                abstract ComposeMap<'S>       : Map<'S,'T>       -> SeqComponent<'S,'U>


ComposeWithMap might be a better name

Tricky naming; they way the functions work out is that it is ComposeMapWith as map is "first". That might be better... Maybe...

In the midst of watching the AFL grand final so not devoting a huge number of neurones to this, but I think i now agree with your original suggestion...

liboz · 2016-10-01T04:36:10Z

src/fsharp/FSharp.Core/seq.fs

+                inherit SeqComponent<'T,'T>()
+
+                override this.Composer (next:SeqComponent<'T,'V>) : SeqComponent<'T,'V> =
+                    next.ComposeFilter this


It's second.ComposeFilter first in most of the other ones

liboz · 2016-10-01T04:38:08Z

src/fsharp/FSharp.Core/seq.fs

+                    second.ComposeSkip first
+
+                override second.ComposeSkip (first:Skip<'T>) : SeqComponent<'T,'T> =
+                    if System.Int32.MaxValue - second.SkipCount > first.SkipCount then


Seems like this condition would make more sense as Int32.MaxValue > first.SkipCount + second.SkipCount to match expression in the next line.

Done so we don't overflow

liboz · 2016-10-01T04:48:36Z

src/fsharp/FSharp.Core/seq.fs

+
+                member __.Filter :'T->bool = filter
+
+            and MapFilter<'T,'U> (map:'T->'U, filter:'U->bool) =


Maybe this should be called MapThenFilter

Sounds good

manofstick · 2016-10-01T04:58:43Z

@liboz

Suck down my repository and start modifying the branch. I'll take in any pull requests you make on that and then they should get transferred back to this main repository.

Go for your life and change whatever you like. I can always modify back of I disagree.

manofstick · 2016-10-02T09:25:39Z

Can someone tell me what the reason why Seq.init (and Seq.initInfinite) lazily call the passed in function on calls to Current (i.e. Current is not populated by MoveNext)? The only valid operation (I can think of...) that can go on without looking at Current is skip?!?!

So we end up with code like:

let n = 
    Seq.init 100 doSomethingExpensive
    |> Seq.skip 50
    |> Seq.sum

Which only calls doSomethingExpensive for the values from 50-99, which to me is already not the intuitive expectation of sequences (i.e. if something mutating is happening in that function then I would be wondering what was going on...) And if I really wanted that to occur I think the following makes much more sense:

let n' = 
    Seq.init 50 ((+) 50)
    |> Seq.map doSomethingExpensive
    |> Seq.sum

Anyway, I assume that I can't change this unintuitive implementation, although no mention of this behaviour is in the documentation? Anyone? Anyone?

And if I can't change it, which I guess I can't, can we at least maybe mark initand initInfinite as deprecated, and maybe make a the more expected functionality available through new create and createInfinite functions?

Hmmm... This should probably be posted where someone will actually read it...

manofstick · 2016-10-04T11:02:13Z

@asik @jackmott

You guys want to help out with this?

Going to sleep myself, going to merge this tomorrow.

jackmott · 2016-10-04T13:00:10Z

@manofstick looks fun! If I get some free time I may dive into this, thanks for the heads up

KevinRansom · 2016-10-07T06:43:04Z

src/fsharp/FSharp.Core/seq.fs

+//        type ISeqEnumerable<'T> =
+//            abstract member Fold<'State> : folder:('State->'T->'State) -> state:'State -> 'State
+
+        module SeqComposer =


have you thought about using a rec module to eliminate all of the and types?

rec module - shiny new feature I haven't used :-)

Personally I don't mind the 'and' syntax, but I'll give it a burl!

KevinRansom · 2016-10-07T06:49:04Z

src/fsharp/FSharp.Core/seq.fs

+            | Finished   = 2
+            | InProcess  = 3
+
+            type SeqComposedEnumerator<'T,'U>(enumerator:IEnumerator<'T>, t2u:SeqComponent<'T,'U>) =


So .... this is a reference type ... have you considered a struct enumerator, it might reduce the gc somewhat?

Hmmm... I'm creating a fair bit of rubbish already; each step in creating the pipeline creates 2 or 3 objects (1 new Enumerable, 1 Factory object and, after the first object, 1 composing object to tie types together) and every time GetEnumerator is called in traverses the factory objects and creates a component object for each level of the pipeline. So unfortunately it's pretty pretty garbage hungry. The Linq libraries recycle the first (and probably only) call to GetEnumerator to reuse the IEnumerable object; possibly something like that could be done to lesson the garbage.

manofstick · 2016-10-07T09:38:28Z

@liboz

OK, my factory change is in; I reran the test from here (I'm modifying that comment to keep track of times in a very superficial way) and it drops the time by ~5% for the 64 bit version; a little less for the 32 bit, but it has also opened up some other possibilities for some more optimizations.

manofstick · 2016-10-07T23:40:07Z

Update in how this is coming along... Given the following small example:

open System.Diagnostics
open System.Linq

[<EntryPoint>]
let main argv = 
    let mutable average = 0.
    let checksum = 19480512987013L

    let data = seq {
        let mutable x = 0L
        while x < 10000000L do
            yield x
            x <- x + 1L }

    for i = 0 to 10 do
        let sw = Stopwatch.StartNew ()

        let total =
            data
            |> Seq.map (fun n -> n * 5L)
            |> Seq.filter (fun n -> n % 7L <> 0L)
            |> Seq.map (fun n -> n / 11L)
            |> fun x -> x.Sum () // nothing up out sleeve

        if total <> checksum then failwith "failed checksum check"

        if i > 0 then
            average <- average + float sw.ElapsedMilliseconds

    printfn "Seq Composer = %f" (average/10.)

    average <- 0.
    for i = 0 to 10 do
        let sw = Stopwatch.StartNew ()

        let total =
            data
             .Select(fun n -> n * 5L)
             .Where(fun n -> n % 7L <> 0L)
             .Select(fun n -> n / 11L)
             .Sum()

        if total <> checksum then failwith "failed checksum check"

        if i > 0 then
            average <- average + float sw.ElapsedMilliseconds

    printfn "Linq = %f" (average/10.)

    0

We get the following results on my machine

version	32-bit (ms)	ratio	64-bit	ratio (ms)
original	1463	0.46	582	0.52
linq	787	0.85	399	0.75
seq composer	669	1.0	301	1.0

So about double the throughput of the original, and a reasonable amount quicker than the Linq implementation, I think we're going along pretty nicely!

!!! _So a call out to anyone who can help finish off the whole of the surface area_ !!!

7sharp9 · 2016-10-11T11:16:24Z

I was curious to how this interacts with the state machine compilation of sequences in the compiler?

https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/LowerCallsAndSeqs.fs#L68

manofstick · 2016-10-11T15:45:48Z

@7sharp9
Independent? Unless I'm missing something? I.e. that code is referring to the computation seq expression, but this is just the Seq module. They don't reference the same function names so code gen wouldn't be affected, although obviously you can consume a seq from within, but this PR doesn't change the external behavior... But maybe you are referring to something more specific?

manofstick · 2016-10-11T20:00:17Z

@7sharp9
That said, I think there could be some potential opportunity if the seq computational expressions became SeqComposer aware (possibly derived from). But I'm still 86 functions away from completing this, so I'm guessing I'm not going to be in that space for a while! (Any help with the remaining functions would be welcome!)

7sharp9 · 2016-10-11T20:05:16Z

@manofstick I was just thinking out loud as usual, just wondered if any change here would cause AST difference in the seq compilation stuff.

I would help if I knew how to, and also if I wasn't working on a little addin for vscode to show F# signatures for dlls's.

manofstick · 2016-10-11T20:12:09Z

@7sharp9

No worries! Talk is good; these PRs are usually quite lonely places and I usually end up just talking to myself (maybe somebody listens I'm not sure; make it a bit hard to know if I'm just off in total crazy land of not!)

And if you do find yourself bored and looking for something to do then you know where to look :-)

manofstick · 2016-10-12T18:38:34Z

@KevinRansom

Do you want to be informed when intermittent failures occur within the build? Now it might be my fault, but it could also just be a race condition, which is what the test says it's about, but I assume that's bad that it failed; I.e. whatever steps that are being taken to avoid the race condition are failing. Anyway, as said, maybe this will occur again and is my fault:

Failed : FSharp.Core.Unittests.FSharp_Core.Microsoft_FSharp_Control.AsyncModule.OnCancel.RaceBetweenCancellationHandlerAndDisposingHandlerRegistration
Expected: True
But was: False
at <StartupCode$FSharp-Core-Unittests>.$AsyncModule.test@286-22(Unit unitVar0) in C:\projects\visualfsharp-3dtit\src\fsharp\FSharp.Core.Unittests\FSharp.Core\Microsoft.FSharp.Control\AsyncModule.fs:line 299
at FSharp.Core.Unittests.FSharp_Core.Microsoft_FSharp_Control.AsyncModule.OnCancel.RaceBetweenCancellationHandlerAndDisposingHandlerRegistration() in C:\projects\visualfsharp-3dtit\src\fsharp\FSharp.Core.Unittests\FSharp.Core\Microsoft.FSharp.Control\AsyncModule.fs:line

manofstick · 2016-10-13T18:59:31Z

@PatrickMcDonald @mexx

Given your previous work on Seq related things I am wondering if have any time to help either review or contribute to this PR. Basically it's a more efficient pipeline combining Seq calls which results in better performance.

liboz · 2016-10-14T03:08:42Z

We should probably discuss which functions we want to actually implement. I think I picked many of the easier ones, some of the remaining ones may be more difficult.

Do we even want to implement the functions that consume seqs other than fold, which you basically have a special case for? Maybe we want to use fold to implement some of the other functions like sum/sumby and things of that nature or do we want to keep it as it currently is where those things are implemented by themselves?

manofstick · 2016-10-14T03:31:32Z

@liboz
But I'll give it some thought over the weekend.

My plan was to add an iter at the enumerable level and then try to implement everything off that, including fold which had really just been done as a proof of concept. But maybe this won't be suitable performance.

My plan before that was to implant concat by tweaking my append implementation.

I haven't really given too much thought beyond those; but I guess I would like to get 100% coverage as leaving the composer more expensive than remaining within.

manofstick · 2016-10-14T23:24:56Z

Fairly often we are getting a build error, but not of our making:

1) Failed : FSharp-Tests-Core+Members+Incremental.incremental(FSI_FILE)
Error running command 'C:\projects\visualfsharp-3dtit\tests\..\release\net40\bin\fsiAnyCPU.exe' with args ' -r:System.Core.dll --nowarn:20 --define:INTERACTIVE --maxerrors:1 --abortonerror test.fsx' in directory 'C:\projects\visualfsharp-3dtit\tests\fsharp\core\members\incremental'. ERRORLEVEL -1073740771 ERRORLEVEL -1073740771
at NUnitConf.checkTestResult(Result`2 result) in C:\projects\visualfsharp-3dtit\tests\fsharp\nunitConf.fs:line 14

This has been commented upon many times.

liboz · 2016-10-14T23:29:19Z

Is there a reason you remove the extra machinery for Map2? On my computer it makes ~5% difference in speeds (~700 vs ~660 for 64bit).

This was my test script:


open System.Diagnostics
open System.Linq

let average a = 
    let sum = Array.sum a
    sum/a.LongLength

[<EntryPoint>]
let main argv = 
    let data = seq {
        let mutable x = 0L
        while x < 10000000L do
            yield x
            x <- x + 1L }

    let times = Array.zeroCreate 10

    let work () = 
        data
        |> Seq.map (fun n -> n * 5L)
        |> Seq.filter (fun n -> n % 7L <> 0L)
        |> Seq.map (fun n -> n / 11L)
        |> Seq.map2 (fun i j -> j % i) [|1L..10000000L|]
        |> Seq.sum

    printfn "warmup %A" (work())

    for i = 1 to 10 do
        let sw = Stopwatch.StartNew ()
        let mutable total = 0L

        total <- work()

        printfn "%d (%d)" sw.ElapsedMilliseconds total
        times.[i-1] <- sw.ElapsedMilliseconds
    printfn "Average time was %d" (average times)
    0

manofstick · 2016-10-14T23:43:59Z

@liboz

When I was adding map3 I thought I would just simplify; but oops, I should have removed the other one - as when piping it is the second argument that is added to the chain (or just left them both in).. maybe can you reinstate please? I'm off on looking-after-the-kids duty...

- also renamed delayed as delay to match seq

sorts/rev/permute

- and moved creating Value comparer to a helper function

- more consistency formatting

KevinRansom · 2017-02-28T20:54:39Z

@manofstick

Thanks for looking into this, I'm closing this for now. If you get any ideas about how to proceed feel free to re-open it.

Kevin

mrange · 2017-02-28T21:05:47Z

This is just IMO but I felt this PR to be very important. Seq for me is discouragingly slow and I think it might hurt perception of F# that the default lazy sequence in F# is not performing as well as it could be (especially Seq.init)

I realize this is a complex and risky PR but I am of the opinion that the performance benefits of this PR is so significant that it should be scheduled for inclusion somehow.

I suggested above that this maybe shouldn't be a drop-in-replacement of Seq (which is very scary) but instead could be shipped as a SeqModule. When the issues have been ironed out in the wild it can replace or deprecate Seq. While this is unattractive it's better IMO then SeqModule never getting merged.

PS.
I asked how to help reviewing it above but I would need some documentation on the general principles of SeqModule

cloudRoutine · 2017-02-28T21:18:26Z

@KevinRansom were waiting for you or @dsyme to review it but neither of you got around to it. The checklist at the top of this PR isn't accurate, the entire Seq module has been implemented in terms of composers.

Is there something preventing this from being a candidate for 4.2?

KevinRansom · 2017-02-28T21:20:20Z

@cloudRoutine
In that case I should re-open it. My apologies.

Kevin

manofstick · 2017-02-28T21:38:06Z

Sorry, time, time, time, what has become of me... :-) (Been a little distracted up in dotnet/coreclr#8963 - which funnily enough, was due to @mrange and this PR... But even then still struggling to find time between work and family...)

Anyway, this PR is "pretty good", but does still need polishing a bit, but definitely in the stage where a review on the official side would be warranted/nice/necessary...

KevinRansom · 2017-02-28T21:52:16Z

@manofstick @cloudRoutine @dsyme

Please don't feel pressured by my polling these PR's from time to time I'm just trying to figure out which ones people still care about. There is nothing like closing one to encourage people to remind me how important they are :-) ...

For this one because it so big and central to the language and library we need a couple of plans ...

1. Review additional public APIs. --- There are a ton of new public APIs in FSharp.Core ... well about 20.
--- TBH toComposer and Composer.Core do not jump out at me and say what they are and why I should use them, and so I would start there, is Composer the most usable name .... of course they may be exactly the right name in which case you need to educate me, and probably many .net developers why it's the right name.

The fsharplang repo is where that kind of thing typically happens.

2. We need to determine whether this is a suitable plug-in replacement for the Seq module. We probably need someone in the community to put together a suite of Seq tests and using the current Seq module and then replace it with this ... and verify incompatibilities.

@manofstick , @cloudRoutine you shouldn't feel like you need to be the developer to do this testing, indeed what we do here is teamwork so I would hope someone who wants to get started on contributing to F# could lead that effort.

I will mark this as F# 4.2 for now ...

dsyme · 2017-02-28T21:59:45Z

@cloudRoutine My comments here still pretty much apply.

It's currently red. We need it green before we can review.
It's huge. There are sure to be bugs. I can't even begin to think how we'd review for correctness. Line by line I suppose, with many more comments. (I'd like to see code covereage w.r.t. tests too, but we don't have anything in pllace for that).
It's novel. Every abstraction feels new to me, though I guess I'll get the hang of them

But the performance gains are good. So I'll wait until it's green if that's ok and then I'll take a look. But it's really a monster PR in one of the most critical modules w.r.t. correctness :)

p.s. There are whitespace changes, e.g. seq.fsi it would be good if they can be removed and the diff minimized

cloudRoutine · 2017-03-02T15:49:26Z

@manofstick I've been trying to rebase and squash this PR and it's been a real disaster. I'm encountering conflicts on lots of files that are unrelated to the work on this PR, and trying to disentangle everything has gone quite poorly.

As most of the history is churn related to the implementation and deals with constructs and approaches that were ultimately abandoned or replaced it might be easier to make a new PR off of the current master with the same basic changes to have a sane history, reasonable amount of commits, etc.

Or we can just merge master straight in, resolve that the past is history so it might as well stay a mystery and keep on trucking, but I don't feel great about that.

rmunn · 2017-03-03T17:29:55Z

I'd be in favor of the "create a new PR from the current state and close this one" approach, though I'd say that this one shouldn't be closed until the new PR has been able to be tagged "Candidate for F# 4.2". The series of commits in this PR has quite a bit of value as a history of how the Seq Composer feature was developed and how the design shifted over time, but it's too long to do anything useful with now. I'd say let this PR become a historical archive, and create a new PR to carry on with development and code reviews.

manofstick · 2017-03-03T22:27:15Z

OK; well I'll spawn a new branch from current master and go from there. I agree it will make things a lot cleaner; although it does make me a little sad to remove other peoples contributions from the source tree. I'll try and get a baseline green build up this weekend; but that depends on how nice my children are to me...

So everyone who's involved happy with that?

cloudRoutine · 2017-03-03T23:29:27Z

I don't mind losing a few commits. the functionality is all that matters to me 😉

manofstick · 2017-03-05T03:00:54Z

@smoothdeveloper has rebased this over at #2526. That is where I'm heading to see if I can bash this sucker into a green build... (I'm assuming this is only generated il output and surface area checks; which to me should really constitute a different layer of "testing", as they shouldn't stop don/kevin/etc's reviews [and review isn't even really the right world; input would be better, which I think I had requested at earlier stages, but anyway, hack on!] - but that's a whole different issue!)

dsyme · 2017-03-24T14:45:39Z

Closing because #2587 exists

msftclas added the cla-already-signed label Sep 28, 2016

liboz reviewed Oct 1, 2016

View reviewed changes

KevinRansom reviewed Oct 7, 2016

View reviewed changes

manofstick added 8 commits December 28, 2016 14:04

Fixed constraint (applied on wrong argument)

36ad648

Added module for GroupBy

84a107c

- also renamed delayed as delay to match seq

Moved Array based function to Composer

0b19340

sorts/rev/permute

ToArray via Fold

f5461ea

- and moved creating Value comparer to a helper function

null checks handled in toComposer

9d35c95

countBy

8e5ce24

Remove comented minValBy/maxValBy

4b8c858

head/last into Composer

7831325

- more consistency formatting

manofstick mentioned this pull request Jan 17, 2017

Alternative System.Lazy implementation dotnet/coreclr#8963

Merged

7 tasks

KevinRansom closed this Feb 28, 2017

KevinRansom reopened this Feb 28, 2017

KevinRansom added the Candidate for F# 4.2 label Feb 28, 2017

This was referenced Mar 6, 2017

[WIP] Manofstick seq composer squashed #2526

Closed

[WIP] iseq. A fast seq implementation. #2587

Closed

dsyme closed this Mar 24, 2017

dsyme removed the Candidate for F# SDK version 4.5 label Mar 12, 2018

manofstick mentioned this pull request Oct 2, 2019

Added Cistern/System Linq plus change to array agocke/fast-enumerable#1

Open


		abstract Composer : SeqComponent<'U,'V> -> SeqComponent<'T,'V>

		abstract ComposeMap<'S> : Map<'S,'T> -> SeqComponent<'S,'U>


		member __.Filter :'T->bool = filter

		and MapFilter<'T,'U> (map:'T->'U, filter:'U->bool) =

[WIP] Seq Composer #1570

[WIP] Seq Composer #1570

Conversation

manofstick commented Sep 28, 2016 • edited Loading

manofstick commented Sep 30, 2016

manofstick commented Sep 30, 2016

liboz commented Sep 30, 2016

manofstick commented Sep 30, 2016

dsyme commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manofstick commented Oct 1, 2016

manofstick commented Oct 2, 2016

manofstick commented Oct 4, 2016

jackmott commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manofstick commented Oct 7, 2016

manofstick commented Oct 7, 2016

7sharp9 commented Oct 11, 2016

manofstick commented Oct 11, 2016

manofstick commented Oct 11, 2016

7sharp9 commented Oct 11, 2016

manofstick commented Oct 11, 2016

manofstick commented Oct 12, 2016 • edited Loading

manofstick commented Oct 13, 2016

liboz commented Oct 14, 2016

manofstick commented Oct 14, 2016 • edited Loading

manofstick commented Oct 14, 2016

liboz commented Oct 14, 2016 • edited Loading

manofstick commented Oct 14, 2016

KevinRansom commented Feb 28, 2017

mrange commented Feb 28, 2017

cloudRoutine commented Feb 28, 2017

KevinRansom commented Feb 28, 2017

manofstick commented Feb 28, 2017

KevinRansom commented Feb 28, 2017

dsyme commented Feb 28, 2017

cloudRoutine commented Mar 2, 2017

rmunn commented Mar 3, 2017

manofstick commented Mar 3, 2017

cloudRoutine commented Mar 3, 2017

manofstick commented Mar 5, 2017

dsyme commented Mar 24, 2017

manofstick commented Sep 28, 2016 •

edited

Loading

manofstick commented Oct 12, 2016 •

edited

Loading

manofstick commented Oct 14, 2016 •

edited

Loading

liboz commented Oct 14, 2016 •

edited

Loading