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

[WIP] Fix #513 for recursive types #961

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@manofstick
Contributor

manofstick commented Feb 12, 2016

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)

Paul Westcott and others added some commits Feb 3, 2016

Fix recursive types for equality and comparison
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
@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Feb 12, 2016

Hi @manofstick, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

msftclas commented Feb 12, 2016

Hi @manofstick, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

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 =

This comment has been minimized.

@dsyme

dsyme Feb 12, 2016

Contributor

Please use a proper uppercase name for this module. What is "mos"?

@dsyme

dsyme Feb 12, 2016

Contributor

Please use a proper uppercase name for this module. What is "mos"?

@@ -1871,7 +1871,12 @@ namespace Microsoft.FSharp.Core
module mos =
type IGetType =

This comment has been minimized.

@dsyme

dsyme Feb 12, 2016

Contributor

Looking through all this code I realize now that #513 really was not properly code reviewed at all. GitHub simply didn't show the diff for the critical file prim-types.fs and no comments or proper review has been done. That this happened is is a grave problem with relying on GitHub PRs for code review.

For example two basic things that indicate this code hasn't been reviewed:

  • We need proper /// comments on every type declaration in prim-types
  • Please don't use abc_def_hij style naming

I'm not sure what to do about this yet. We should really revert the commit and start again with a full code review, but I think that @KevinRansom didn't want to do that. I do understand that the testing is good and the performance results are real. But we need to be more considered here.

@dsyme

dsyme Feb 12, 2016

Contributor

Looking through all this code I realize now that #513 really was not properly code reviewed at all. GitHub simply didn't show the diff for the critical file prim-types.fs and no comments or proper review has been done. That this happened is is a grave problem with relying on GitHub PRs for code review.

For example two basic things that indicate this code hasn't been reviewed:

  • We need proper /// comments on every type declaration in prim-types
  • Please don't use abc_def_hij style naming

I'm not sure what to do about this yet. We should really revert the commit and start again with a full code review, but I think that @KevinRansom didn't want to do that. I do understand that the testing is good and the performance results are real. But we need to be more considered here.

wrapperTypeDef.MakeGenericType [| ty |]
else
let wrapperTypeDef = typedefof<EssenceOfEqualsWrapper<int,EqualsTypes.Int32>>
wrapperTypeDef.MakeGenericType [| ty; comp |]
Activator.CreateInstance wrapperType
type u = EqualsTypes.Int32

This comment has been minimized.

@dsyme

dsyme Feb 12, 2016

Contributor

Please don't use abbreviations like this

@dsyme

dsyme Feb 12, 2016

Contributor

Please don't use abbreviations like this

@manofstick

This comment has been minimized.

Show comment
Hide comment
@manofstick

manofstick Feb 14, 2016

Contributor

Not longer makes sense after #966

Contributor

manofstick commented Feb 14, 2016

Not longer makes sense after #966

@KevinRansom

This comment has been minimized.

Show comment
Hide comment
@KevinRansom

KevinRansom May 26, 2016

Contributor

@manofstick @dsyme

Can you offer some guidance of what needs to happen for this PR?

Thanks

Kevin

Contributor

KevinRansom commented May 26, 2016

@manofstick @dsyme

Can you offer some guidance of what needs to happen for this PR?

Thanks

Kevin

@manofstick

This comment has been minimized.

Show comment
Hide comment
@manofstick

manofstick May 26, 2016

Contributor

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...

Contributor

manofstick commented May 26, 2016

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...

@KevinRansom

This comment has been minimized.

Show comment
Hide comment
@KevinRansom

KevinRansom Jun 24, 2016

Contributor

@manofstick No rush mate, I just wondered if it was still an active PR.

Kevin

Contributor

KevinRansom commented Jun 24, 2016

@manofstick No rush mate, I just wondered if it was still an active PR.

Kevin

@KevinRansom

This comment has been minimized.

Show comment
Hide comment
@KevinRansom

KevinRansom Aug 1, 2016

Contributor

@manofstick
If you want for this to make F# 4.1, I am afraid I am going to need an updated PR pretty soon.

Contributor

KevinRansom commented Aug 1, 2016

@manofstick
If you want for this to make F# 4.1, I am afraid I am going to need an updated PR pretty soon.

@manofstick

This comment has been minimized.

Show comment
Hide comment
@manofstick

manofstick Aug 1, 2016

Contributor

@KevinRansom

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,
@manofstick

Contributor

manofstick commented Aug 1, 2016

@KevinRansom

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,
@manofstick

@KevinRansom

This comment has been minimized.

Show comment
Hide comment
@KevinRansom

KevinRansom Aug 6, 2016

Contributor

It's all cool mate, and thanks for taking care of this.

Kevin

Contributor

KevinRansom commented Aug 6, 2016

It's all cool mate, and thanks for taking care of this.

Kevin

@manofstick

This comment has been minimized.

Show comment
Hide comment
@manofstick

manofstick Aug 14, 2016

Contributor

@KevinRansom @dsyme

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:

type IStructuralEquatable<'a> = // naming is open for discussion, I'm just being lazy
    inherit IStructuralEquatable //maybe; but maybe not even desirable
    abstract Equals : other:'a -> comparer:IEqualityComparer<'a> -> depth:int -> bool
    abstract GetHashCode : comparer: IEqualityComparer<'a> -> depth:int -> int

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:

  • Added complexity
  • using types from previous compiler builds wouldn't implement this interface.

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.

Contributor

manofstick commented Aug 14, 2016

@KevinRansom @dsyme

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:

type IStructuralEquatable<'a> = // naming is open for discussion, I'm just being lazy
    inherit IStructuralEquatable //maybe; but maybe not even desirable
    abstract Equals : other:'a -> comparer:IEqualityComparer<'a> -> depth:int -> bool
    abstract GetHashCode : comparer: IEqualityComparer<'a> -> depth:int -> int

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:

  • Added complexity
  • using types from previous compiler builds wouldn't implement this interface.

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.

@manofstick

This comment has been minimized.

Show comment
Hide comment
@manofstick

manofstick Oct 17, 2016

Contributor

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:

Classes for the Masses

Contributor

manofstick commented Oct 17, 2016

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:

Classes for the Masses

@manofstick

This comment has been minimized.

Show comment
Hide comment
@manofstick

manofstick Oct 18, 2016

Contributor

@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...

Contributor

manofstick commented Oct 18, 2016

@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 dsyme changed the title from Fix #513 for recursive types to [WIP] Fix #513 for recursive types Nov 22, 2016

@KevinRansom

This comment has been minimized.

Show comment
Hide comment
@KevinRansom

KevinRansom Dec 14, 2016

Contributor

@manofstick @dsyme

Can you offer some guidance about what we should do with this PR?

Thank you

Kevin

Contributor

KevinRansom commented Dec 14, 2016

@manofstick @dsyme

Can you offer some guidance about what we should do with this PR?

Thank you

Kevin

@manofstick

This comment has been minimized.

Show comment
Hide comment
@manofstick

manofstick Dec 15, 2016

Contributor

@KevinRansom

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.

Contributor

manofstick commented Dec 15, 2016

@KevinRansom

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.

@MattWindsor91

This comment has been minimized.

Show comment
Hide comment
@MattWindsor91

MattWindsor91 Dec 15, 2016

@manofstick, @KevinRansom

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)…

MattWindsor91 commented Dec 15, 2016

@manofstick, @KevinRansom

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)…

@KevinRansom

This comment has been minimized.

Show comment
Hide comment
@KevinRansom

KevinRansom Feb 13, 2017

Contributor

@manofstick @MattWindsor91

Please feel free to reopen this PR ... if you decide to pick it back up again.

Thanks for doing this

Kevin

Contributor

KevinRansom commented Feb 13, 2017

@manofstick @MattWindsor91

Please feel free to reopen this PR ... if you decide to pick it back up again.

Thanks for doing this

Kevin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment