-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding ListEnumerable, Choose, and some cleanup #1
Adding ListEnumerable, Choose, and some cleanup #1
Conversation
I had tried a common based type, but it had decreased performance a bit (not much!), and as that was what I was trying to maximize I decided the duplication wasn't too bad. But I'll suck this in and see how it goes. Did you do any measuring around using list directly rather than just using is enumerable interface? (Have to wait for tomorrow, as it's 2am for me and there is a merge conflict...) |
Alright I did some measurements for using the interface or not. It seems to make no difference effectively (except for the list one which is new), probably because of JIT optimizations. I included tests using Linq instead and it seems that we lose out to Linq (which composes Where/Select) for smaller collection sizes and on 32bit always. However, we seem to be better on larger collections on 64bit. Here's my test code, which basically does a map, a filter, another map, then a skip. The "Test" implementation were made copying the code before my PR and renaming it. Host Process Environment Information: Type=map Mode=Throughput LaunchCount=5
|
Not sure if the gist is correct? interfaceArrayTest/normalArrayTest and linterfaceSeqTest/normalSeqTest both look identical. in normalListTest you are using Seq.xxxTest functions though? (I'll merge the stuff in when I get home tonight) |
Well that's unfortunate. Time to run it again! At least this confirms the list thing works well haha. |
Attempt #2. It seems the Interface performance about the same as normal, maybe even slightly faster on 64bit. But on the LegacyJit, it's completely horrible. I'll investigate a bit more into it tomorrow.
|
I ponder if your test is running correctly. I get similar performance to the Linq libraries, vastly better than existing Seq. |
Not saying that it is, but maybe you should try before your changes. |
I'm cloning your repository, and having a bit of a play around to see if that is what is negatively affecting performance (from your test suite). So sorry for any delays in merging this PR. |
OK; I'm not going to pull this PR in because it negatively affects performance; although not to the scale that you were seeing in your test suite; so I'm not sure what is going on there. So if I ran the following code, linking to an original FSharp.Core, your PR's FSharp.Core and my PR's FSharp.Core
And I also ran the following for Linq comparison:
Post Factory changes
These numbers do jump around a little from run to run, but not much. You can see that your changes had a negative affect on the 32-bit version. And you can see that the PR give basically the same performance as the linq version. OK, this is for very large amounts of data, lets try with the very small:
Linq clearly wins the day here. We could modify the code to minimize object creation, which is what the linq stuff does (i.e. shares enumerable/enumerator for first instance; i create more intermediate state combinations...) But really lots of small ones are not really my interest; times are pretty fast in those cases anyway... Post unbox.any changes:
Post factory changes:
|
Yea, running your test gets me similar results, though your test suite should probably do a JIT warmup, so the first iteration isn't a decent amount slower than the other ones. It appears the interface makes no difference on 64bit, but is about 30% slower on 32bit. I wonder what crazy thing happened with BenchmarkDotNet on 32bit though... I will rebase this PR and make it so that it only has the ListEnumerator and Choose instead of the interface stuff. Also on another note, I think one major reason we do worse than Linq for small collections is due to the explicit upcast in F# resulting in extra unboxing and that effect dominating the performance for small collections. For small collections, that's probably a 30% cost. See here: dotnet#1469 |
I put in a fix to remove the unbox.any (sorry it was before I saw you had updated this, so back to conflict, if you could fix that would be great) So now the performance on short seqs I'd better; still not as good as Linq library though. |
…o manofstick # Conflicts: # src/fsharp/FSharp.Core/seq.fs
@manofstick did the merge. I think the remaining performance difference is from the reusing the same object that Linq does, which I'm not sure we want to deal with. |
Looking a bit closer, since I didn't see your test suite, I imagine there is also a hit in performance for upcasting the |
I was just running the test from the other day (I.e the one where the Linq libraries smashed us for performance) The upcasting of the SeqComponents is OK as it is class based inheritance. The f# compiler only seems to insert unbox.any if you are upcasting to an interface. I assume this is to correctly handle the car where an interface is implemented on a value type, but when I asked about why it did it previously all I heard was the crickets... All being well, when I get home tonight I'll merge this PR in. In the parent PR I have made a list of all the Seq functions that exist, so if you want to hack away at any more, go for your life, just put a comment on whatever ones your taking. And maybe create individual PRs for each? |
Sounds good, I'll try to get a few of them soon. |
Here's some stuff. Not super convinced about all the name choices especially things like
ComposeWithMapThenFilter
could be misinterpreted on reading.I implemented the common interfaces for the enumerable in an abstract class, since it seems like all those things are just copy/paste. I made an abstract class for the enumerator for the interfaces, which again seem all the same.