Skip to content
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

Avoid calling CanMemberSigsMatchUpToCheck twice #1685

Merged
merged 3 commits into from
Nov 7, 2016
Merged

Avoid calling CanMemberSigsMatchUpToCheck twice #1685

merged 3 commits into from
Nov 7, 2016

Conversation

gusty
Copy link
Contributor

@gusty gusty commented Oct 30, 2016

By upgrading the undo trace to an undo/redo trace we may avoid calling CanMemberSigsMatchUpToCheck twice in a common scenario.

If the second call to FilterEachThenUndo (the one for applicable methods) was performed then we can collect the effects of the call to CanMemberSigsMatchUpToCheck and avoid calling it again in the final checks.
The only difference between these two calls is the alwaysCheckReturn parameter which in the final checks must always be true, but that check can be performed by-hand after re-playing the trace.

Here's a crazy-compile-time-test with the following results:

val compileTime : float = 453586.2157  // before this PR (but after #1530)
val compileTime : float = 14454.7689   // after this PR

So in this case it will speed up 30x but it's clear that the speed up is exponential, so it will depend on the level of nesting.

For simple and no nested cases it's expected at least a 90% of the original time.

No other effect is expected since the modified code is equivalent to the original.

@forki
Copy link
Contributor

forki commented Oct 30, 2016

Not sure if this is correct or working for real but I just wanted to say 💋

@gusty
Copy link
Contributor Author

gusty commented Oct 31, 2016

Thanks @forki I can confirm you that this is really working !
I still need to take the time to do more benchmarks, but I can tell you that yesterday did play a bit after merging all my PRs and found out that experimental code I've written 5 years ago like this now it just works fine. Before it was taking insane compile times and the tuple size was bounded to 4 or 5 elements, now I can use even 8-uples and it just come back in milliseconds.
So code like that is now "usable" ;)
When I have time I will do more benchmarks, including each PR.

@dsyme
Copy link
Contributor

dsyme commented Oct 31, 2016

@gmpl Super interesting, and the technique looks sound and otherwise non-invasive to me. Can't immediately tell why the speedup is exponential though - could you explain that more?

A bit more documentation would be good, and move the "replay the trace" line to a trace.Replay call may make sense (so actions is only access in the type itself)

@gusty
Copy link
Contributor Author

gusty commented Oct 31, 2016

Thanks @dsyme.
I believe the speed-up in this and the other PRs (maybe not for #1650) is exponential, because for instance here CanMemberSigsMatchUpToCheck may internally call ResolveOverload and so it will be called recursively.

It all depends if by resolving an overload 'x' you need to resolve an overload 'y'.
Before #1530 it was even more noticed the exponential time, since it was nesting to solve the same constraint, now it's better but it depends on the client code, the example I show has nested overloads.

@KevinRansom KevinRansom merged commit 36c7e0f into dotnet:master Nov 7, 2016
@KevinRansom
Copy link
Member

@gmpl Thank you for taking care of this.

Kevin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants