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

Span for .. in .. do optimization #6213

Merged
merged 27 commits into from
Apr 16, 2019
Merged

Span for .. in .. do optimization #6213

merged 27 commits into from
Apr 16, 2019

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Feb 8, 2019

Resolves this issue: #6195

Optimizes for .. in .. do on span types by using the indexer instead of the enumerator.

Remaining work:

  • Add ReadOnlySpan optimization
  • Tests

@TIHan TIHan changed the title Span for .. in .. do optimization [WIP] Span for .. in .. do optimization Feb 8, 2019
@cartermp
Copy link
Contributor

cartermp commented Feb 8, 2019

Make it snappy

Copy link
Contributor

@forki forki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only small nitpick. Good work. Thx

src/fsharp/TastOps.fs Show resolved Hide resolved
src/fsharp/TastOps.fs Outdated Show resolved Hide resolved
src/fsharp/TastOps.fs Outdated Show resolved Hide resolved
@TIHan TIHan changed the base branch from dev16.1 to dev16.0 February 11, 2019 20:41
@TIHan TIHan changed the base branch from dev16.0 to dev16.1 March 11, 2019 17:53
@TIHan
Copy link
Contributor Author

TIHan commented Mar 11, 2019

This is almost done, trying to get tests to pass under .net core.

@@ -16,11 +16,6 @@
<Compile Include="HashIfExpression.fs" />
<Compile Include="ProductVersion.fs" />
<Compile Include="EditDistance.fs" />
<Compile Include="Compiler.fs" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to remove these, or is this temporary? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe so. I moved these tests to FSharp.Tests.FSharpSuite instead.

@TIHan
Copy link
Contributor Author

TIHan commented Mar 13, 2019

This is ready I believe.

@TIHan TIHan changed the title [WIP] Span for .. in .. do optimization Span for .. in .. do optimization Mar 13, 2019
@TIHan TIHan changed the base branch from dev16.1 to master April 5, 2019 19:52
@TIHan
Copy link
Contributor Author

TIHan commented Apr 5, 2019

I would like to get this into master asap since it fixes tests as well.

src/fsharp/TastOps.fs Outdated Show resolved Hide resolved
src/fsharp/TypeChecker.fs Outdated Show resolved Hide resolved
src/fsharp/TypeChecker.fs Outdated Show resolved Hide resolved
@TIHan
Copy link
Contributor Author

TIHan commented Apr 11, 2019

If tests pass, this should now be completely ready.

@TIHan
Copy link
Contributor Author

TIHan commented Apr 11, 2019

This is really really ready. :)

Just needs a review.

@KevinRansom
Copy link
Member

@dsyme, could you run your eyes over this one last time please.

Thanks

Kevin

@KevinRansom KevinRansom merged commit 08f6169 into dotnet:master Apr 16, 2019
brettfo added a commit that referenced this pull request Apr 16, 2019
@brettfo
Copy link
Member

brettfo commented Apr 16, 2019

Reverted in #6558 due to a test failure.

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
* Trying to optimize span in for loop

* Added Span_GetItem call

* Almost success with optimization

* Fixed code gen

* Added Span optimization tests

* Added ReadOnlySpan opt

* Cleaning up tests

* Moving tests around

* Trying to figure out span tests

* Trying to fix some tests

* Trying to get some more tests passing

* Fixed range

* Trying to get tests to pass again

* Fixing tests for netcore

* Fix build

* When a solution becomes unloaded, we should clear F#'s cache (dotnet#6420)

* Changing if directives

* Simplifying

* Using a type shape for span optimization

* Fixing one test

* Drastically simplified looking at the type shape for Span

* Simplified a bit more

* RunScript has expected error messages

* Add back net472

* Feedback

* Update SpanOptimizationTests.fs

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

Successfully merging this pull request may close these issues.

6 participants