for expression optimization #219

mrange · 2015-02-06T18:57:15Z

(Moved from codeplex)
'for i in expr do body' optimization

Previously 'for i in expr do body' only had optimization
when the type of expr was an 1D-array.

'for i in expr do body' now has optimizations for when expr
is of type 'string' and 'List`1'.

These optimizations increases the performance of the for
expression but also reduces the number of objects created.

mrange · 2015-02-06T18:59:14Z

src/fsharp/FSharp.Core/prim-types.fsi

@@ -1097,6 +1097,14 @@ namespace Microsoft.FSharp.Core
            //[<CompilerMessage("This function is for use by compiled F# code and should not be used directly", 1204, IsHidden=true)>]
            val inline GetString : source:string -> index:int -> char

+            /// <summary>A compiler intrinsic that gets character from a string</summary>
+            [<CompilerMessage("This function is for use by compiled F# code and should not be used directly", 1204, IsHidden=true)>]
+            val inline GetStringChar : source:string -> index:int -> char


I am looking for alternatives to add these methods to prim-types.fsi. The issue is that in the optimizer there is not so much support for find the correct overloads. In the type-checker it was possible to find the proper overload but migrating that code to the optimizer proved difficult.

mrange · 2015-02-06T19:00:36Z

Just wanted to get the PR started again.

enricosada · 2015-02-09T18:22:22Z

src/fsharp/FSharp.Core/list.fs

@@ -594,4 +594,4 @@ namespace Microsoft.FSharp.Collections
        let truncate count list = Microsoft.FSharp.Primitives.Basics.List.truncate count list

        [<CompiledName("Unfold")>]
-        let unfold<'T,'State> (f:'State -> ('T*'State) option) (s:'State) = Microsoft.FSharp.Primitives.Basics.List.unfold f s
+        let unfold<'T,'State> (f:'State -> ('T*'State) option) (s:'State) = Microsoft.FSharp.Primitives.Basics.List.unfold f s


useless whitespace change. Also newline at end of file is good

dsyme · 2015-02-09T18:44:28Z

I think there's a problem with this PR when using FSharp.Core 4.3.0.0 or 4.3.1.0

The F# 4.0 compiler must be able to successfully work with FSHarp.Core 4.3.0.0 and 4.3.1.0. The GetStringChar and GetStringLength intrinsics won't be available in those libraries (TryDeref on the ValRef will return None). For those cases, we need to make sure the optimization doesn't apply

@KevinRansom @latkin - This reminds me that we really need to beef up our testing for this backward-library-targeting case. I wonder if the "single file test and run" in the "fsharp" suite needs to be beefed up with a compilation against both 4.3.0.0 and 4.3.1.0, with a #define to cover the API differences between the libraries.

latkin · 2015-02-12T21:14:02Z

Yes, when reviewing this on Codeplex, it got to a good state for F# 4.0-only. But design causes the 4.0 compiler to error out when it encounters one of these loops and is targeting 3.1.0 or earlier. This needs to be resolved.

@dsyme yes, this back-compat stuff is on my backlog. I have some ideas, just need to find time.

dsyme · 2015-02-13T10:27:11Z

@mrange - I think you just need to add mkstringchar_vref to TcGlobals and check mkstringchar_vref.CanDeref as part of the case guard here: https://github.com/Microsoft/visualfsharp/pull/219/files#diff-b6f3f390e9807a7e9d93964104d74363R7819

Likewise for any other cases using new constructs

mrange · 2015-03-08T23:13:25Z

Hi.

Reworked the code so that it no longer relies on new functions being added to IntrinsicFunctions.

That should make the change backwards comptatible with FSharp.Core 4.3.0.0 and 4.3.1.0.

It's getting a bit late here so hopefully I haven't lost latkins portable fixes along the way.

mrange · 2015-03-09T06:11:57Z

AppVeyor seemed to break with:
Setting up Visual Studio for debugging extensions. This one-time operation may take a minute or more.
1575 C:\Program Files (x86)\Microsoft Visual Studio 12.0\Common7\IDE\devenv.exe /RootSuffix FSharp /ResetSettings General.vssettings /Embedding /Command File.Exit
1576log4net : error XmlHierarchyConfigurator: Could not create Appender [RollingFileAppender] of type [log4net.Appender.RollingFileAppender]. Reported error follows. [C:\projects\visualfsharp-radou\vsintegration\src\deployment\EnableOpenSource\EnableOpenSource.csproj]
1577 System.IO.FileNotFoundException: Could not load file or assembly 'AWSToolkit.Util' or one of its dependencies. The system cannot find the file specified.
1578 File name: 'AWSToolkit.Util'

Previously 'for i in expr do body' only had optimization when the type of expr was an 1D-array. 'for i in expr do body' now has optimizations for when expr is of type 'string' and 'List`1'. These optimizations increases the performance of the for expression but also reduces the number of objects created. latkin provided the following fixes: 1. Adapting tests to work with core\portable and core\netcore 2. Loop item over strings sometimes uses object, not char 3. Adjustments to debug ranges so that debug stepping behavior is consistent/unchanged 4. Updating codegen tests to represent corrected debug ranges

mrange · 2015-03-10T20:32:27Z

Tried a quick smoke test on 4.3.0.0 and 4.3.1.0 and it seems to work now.

mrange · 2015-03-10T20:33:08Z

Btw, I don't expect this to be merged until after 4.0 is released as I realize this is somewhat risky addition. Yet I want to get it to a mergable state.

mrange · 2015-03-11T21:21:53Z

src/fsharp/tastops.fs

-        Some(spForLoop,elemVar,startExpr,step,finishExpr,bodyExpr,m)
-    | _ -> 
-        None
+let DetectAndOptimizeForExpression g option expr =


The exciting stuff

latkin · 2015-08-06T17:50:39Z

working on validating/merging this guy now

Previously 'for i in expr do body' only had optimization when the type of expr was an 1D-array. 'for i in expr do body' now has optimizations for when expr is of type 'string' and 'List`1'. These optimizations increases the performance of the for expression but also reduces the number of objects created. closes #219 commit e3935d95ce12a28f87e2d77c537b6f18e6d54d98 Author: latkin <latkin@microsoft.com> Date: Thu Aug 6 12:53:35 2015 -0700 Add cross-version test cases commit 1427f418ab1a2fb2b06684fafff809932714cf0a Merge: 01dc508 73509e9 Author: latkin <latkin@microsoft.com> Date: Thu Aug 6 10:42:54 2015 -0700 Merge branch 'pr/for_optimization' of https://github.com/mrange/visualfsharp into mrange-pr/for_optimization Conflicts: src/fsharp/TcGlobals.fs tests/fsharpqa/Source/Optimizations/ForLoop/env.lst commit 73509e9 Author: mrange <marten_range@hotmail.com> Date: Mon Mar 9 00:04:11 2015 +0100 'for i in expr do body' optimization.. Previously 'for i in expr do body' only had optimization when the type of expr was an 1D-array. 'for i in expr do body' now has optimizations for when expr is of type 'string' and 'List`1'. These optimizations increases the performance of the for expression but also reduces the number of objects created. latkin provided the following fixes: 1. Adapting tests to work with core\portable and core\netcore 2. Loop item over strings sometimes uses object, not char 3. Adjustments to debug ranges so that debug stepping behavior is consistent/unchanged 4. Updating codegen tests to represent corrected debug ranges

latkin · 2015-08-07T12:32:39Z

Applied to the OOB branch. Thanks for being patient on this one, it's great to finally have this merged! I added cross-version tests and verified things are good in that regard.

mrange reviewed Feb 6, 2015
View reviewed changes

enricosada reviewed Feb 9, 2015
View reviewed changes

mrange force-pushed the pr/for_optimization branch from f98994f to 9fc834a Compare March 8, 2015 23:07

mrange force-pushed the pr/for_optimization branch from 9fc834a to 1f87057 Compare March 9, 2015 17:11

latkin mentioned this pull request Mar 9, 2015

CI build of EnableOpenSource.csproj is causing failures #300

Closed

mrange force-pushed the pr/for_optimization branch from 1f87057 to 73509e9 Compare March 10, 2015 17:41

mrange reviewed Mar 11, 2015
View reviewed changes

KevinRansom added the V.Next label Apr 7, 2015

latkin mentioned this pull request Apr 22, 2015

Optimize for i in [0 .. 10] do something #377

Closed

latkin added this to the F# 4.0 Update 1 milestone Aug 4, 2015

latkin removed the V.Next label Aug 4, 2015

latkin added the fixed label Aug 7, 2015

latkin closed this Aug 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

for expression optimization #219

for expression optimization #219

mrange commented Feb 6, 2015

mrange Feb 6, 2015

mrange commented Feb 6, 2015

enricosada Feb 9, 2015

dsyme commented Feb 9, 2015

latkin commented Feb 12, 2015

dsyme commented Feb 13, 2015

mrange commented Mar 8, 2015

mrange commented Mar 9, 2015

mrange commented Mar 10, 2015

mrange commented Mar 10, 2015

mrange Mar 11, 2015

latkin commented Aug 6, 2015

latkin commented Aug 7, 2015

for expression optimization #219

for expression optimization #219

Conversation

mrange commented Feb 6, 2015

mrange Feb 6, 2015

Choose a reason for hiding this comment

mrange commented Feb 6, 2015

enricosada Feb 9, 2015

Choose a reason for hiding this comment

dsyme commented Feb 9, 2015

latkin commented Feb 12, 2015

dsyme commented Feb 13, 2015

mrange commented Mar 8, 2015

mrange commented Mar 9, 2015

mrange commented Mar 10, 2015

mrange commented Mar 10, 2015

mrange Mar 11, 2015

Choose a reason for hiding this comment

latkin commented Aug 6, 2015

latkin commented Aug 7, 2015