[WIP] Fix #513 for recursive types #961
Conversation
The new equality and comparison implementation suffered from stack overflow exceptions due to it's lack of use of the IL tail instruction. This fix
|
Hi @manofstick, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
| @@ -1871,7 +1871,12 @@ namespace Microsoft.FSharp.Core | |||
| module mos = | |||
There was a problem hiding this comment.
Please use a proper uppercase name for this module. What is "mos"?
|
Not longer makes sense after #966 |
|
Can you offer some guidance of what needs to happen for this PR? Thanks Kevin |
|
Hiya @KevinRansom, What's needed is for me to do the work to fix it up! @dsyme kindly reviewed it a couple of months back, and created a list of comments which I need to stitch in, but as time passing had moved me on to other projects (and I have been unable to convince my wife to send the children off to an orphanage) I've unfortunately haven't found the time to return to FSharp.Core land for a while. Anyway, from memory, and it is a bit stetchy given a number of passing months, I think I had a bit of a concern with this fix, which was for recursive types (and I think they had to be genericly defined-recursive types [I think concrete types where compiler tail-called them properly]) but came at the expense of removing my eliminate-tail-call idiom in some cases, which then meant that the whole of the project was potentially dubious, as this meant that performance gains for > 64-bit structs was blown out of the water. Maybe I'm not remembering correctly though. Hmmm... I'll have to revisit. Look I'll try to find some time, but I'm pretty stretched (and currently sick!). Do you have any critical deadlines that are looming, so I can be inspired by the fear of missing out... |
|
@manofstick No rush mate, I just wondered if it was still an active PR. Kevin |
|
@manofstick |
|
The vacuum of life must be a Dyson as I have been unable to escape it's grip for the past while! Anyway I definitely haven't abandoned the idea of this pull request, and I think if anything it is much more desirable now given the recent work around extending the ease of creation, as well as extended role, of value types within F# land. So, not that I have given major background brain resources to it, but I was hoping that a better solution to the recursive types issue would pop into my head, but it hasn't. (I.e. there is a dualing battle between eliminating "tail" instructions for > 64bit value types (on 64-bit jit) and blowing the stack on generic types that are then recursively defined.) Possible solution to this is 1) create a calling convention for the 64-bit jit that is tail recursive friendly or 2) create a parallel set of comparison/equality interfaces for values types that pass the type by reference. Now I think 2 is actually a reasonable idea, but massively outside the scope of this work, so my brain has failed me. Anyway, really it's more just a time thing though, as taking my girls to ballet and swimming lessons and the like is the trump card that keeps getting played from my deck. But I like deadlines (it means something is done!), so I'll try to bunker down this weekend. But reality is is probably outside the 4.1 timeframe I'm guessing. Regards, |
|
It's all cool mate, and thanks for taking care of this. Kevin |
|
OK; well still busy so haven't got around to fixing this yet, which, as mentioned in my previous ranting was because I really wasn't too excited by the existing solution that I was proposing, as it seemed a bit of a compromise with my original mission of providing a better solution for value types. (i.e. I was fixing boxing issues, but the > 64 bit tail issue was back, front and center....) Anyway, some neuron finally decided to fire back with a plan which I think will finally fulfill my original vision. It adds a bit more complexity, which I'm not really happy with, as the scope now extends to modifying code generation, but I think to should neatly resolve everything (hahaha famous last words!) OK; well I haven't gone about implementing any of this yet, as I thought I should run it by your guys first. So the problem is that with recursively defined types, because I'm removing "tail", I leave myself open for a recursive spiral down to stack-overflow land. An obviously undesirable state. The solution proposed in this original "fix" was to short circuit my way into my code, restoring some "tail" instructions on the way and thus solving stack-overflow, but now causing some extreme slow downs in cases of > 64bit value types. What I'm now proposing is providing a new interface something along the lines of: Which would be my main point of entry into comparison, which would be implemented "tail" free. When called recursively on types it would increment the depth. After a relatively modest depth traversal (10? 20? 2?) it would fall back to the original comparisons methods (i.e. with boxing, tail and all.) (With possibly also an a derived IEqualityComparer<'a> interface for the comparer with an additional property to determine if we're PER or ER...) Potential issues with this I see are:
Anyway, I'm been hassled for the last 20 minutes to go to a playground, so I haven't proofread this particularly well, but hopefully it is in sufficient detail to get the gist of what I'm thinking of. I'm guessing this is a reasonable amount of work, and given my recent inability to get behind a keyword it could take a while, so if you think it is an unworkable solution, then let me know before I attempt to enter any serious coding effort. Thanks! manofstick. |
|
Ha! Looks like I really should reanimate this at some stage, as looks like the underlying technique has been officially recognized and may make it's way into the compiler: |
|
@CaptainHayashi Not that this PR is in a working state, but this PR (well #513 really) was an actually implementation of the underlying concepts that you are adding syntactic sugar around... |
dsyme
changed the title
|
Can you offer some guidance about what we should do with this PR? Thank you Kevin |
|
Sigh. I really need a week or two solid to face this one again. Well I do ponder if it was an (uncredited!) influence in Classes for the Masses, and if it was then at least it has been useful for something! I did say that this phoenix would rise again and I do hope that I do get inspired to do it, but I see little point of starting from this PRs, and so would probably be better off just forking from master again. I say close it - its doubtful I'll find time within the next few months to get to it - but would be more than happy if someone else grabbed it and polished this mud into a dorodango. |
|
Sorry for not replying earlier, in honesty I've fallen behind on the Classes for the Masses work since ending my MSFT internship. Am interested in theory in picking it back up and seeing it go forwards, but haven't had time recently to do so (or look at this or #513 in depth)… |
|
Please feel free to reopen this PR ... if you decide to pick it back up again. Thanks for doing this Kevin |
The constrained type building blocks model that was used to implement #513 suffered from the fact that the JIT doesn't tail call method calls that are constrained (which was a key ingredient.) This PR short circuits this methods for the specific cases of f# defined recursive types, as defined under generic specifications, which was a regression issue. (although not currently covered by regression tests)