Cleanup Optimizer

[forki]

No description provided.

[forki]

I wonder if FlatList should be removed completely!?

[dsyme]

This diff duplicates code - it doesn't seem right unless we know for sure it gives a measurable performance win.

[forki]

Yeah that idea was stupid. Reverting later

Am 05.09.2016 18:31 schrieb "Don Syme" notifications@github.com:

In src/fsharp/Optimizer.fs
#1519 (comment)
:

•    let vref =
  

•        match tyargs.Length with
  

•        | 2 -> Some cenv.g.generic_compare_withc_tuple2_vref
  

•        | 3 -> Some cenv.g.generic_compare_withc_tuple3_vref
  

•        | 4 -> Some cenv.g.generic_compare_withc_tuple4_vref
  

•        | 5 -> Some cenv.g.generic_compare_withc_tuple5_vref
  

•        | _ -> None
  

•    match vref with
  

•    | Some vref -> Some (DevirtualizeApplication cenv env vref ty tyargs (mkCallGetGenericComparer cenv.g m :: args) m)
  

•    | None -> None
  

•    match tyargs.Length with
  

•    | 2 -> Some (DevirtualizeApplication cenv env cenv.g.generic_compare_withc_tuple2_vref ty tyargs (mkCallGetGenericComparer cenv.g m :: args) m)
  

•    | 3 -> Some (DevirtualizeApplication cenv env cenv.g.generic_compare_withc_tuple3_vref ty tyargs (mkCallGetGenericComparer cenv.g m :: args) m)
  

•    | 4 -> Some (DevirtualizeApplication cenv env cenv.g.generic_compare_withc_tuple4_vref ty tyargs (mkCallGetGenericComparer cenv.g m :: args) m)
  

•    | 5 -> Some (DevirtualizeApplication cenv env cenv.g.generic_compare_withc_tuple5_vref ty tyargs (mkCallGetGenericComparer cenv.g m :: args) m)
  

•    | _ -> None
  

This diff duplicates code - it doesn't seem right unless we know for sure
it gives a measurable performance win.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/Microsoft/visualfsharp/pull/1519/files/28119521a9117c8dd2c8b7552576c56be57fe052..837bfc2b134d02bca14d47789145f2f5ed6ca1b3#r77542728,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNMPGJtTgy1SbVzXIejHwwLjJ4XJtks5qnEPcgaJpZM4J1Jn3
.

[forki]

ready for review

[KevinRansom]

@forki, I actually prefer the when clause on this pattern match it eliminates the duplication.

Does that make sense?

Kevin

[forki]

yeah but then you pattern match twice

[KevinRansom]

looks good to me.

[dsyme]

One timeout failure on AppVeyor, unrelated to PR:

[00:45:27] 1) Failed : FSharp.Core.Unittests.FSharp_Core.Microsoft_FSharp_Control.AsyncModule.AwaitWaitHandle.Timeout
[00:45:27]   Delta is too big 969
[00:45:27]   Expected: True
[00:45:27]   But was:  False

[dsyme]

I prefer the original immutable code here. TMDefRec is relatively rare so I'm certain this doesn't dominate CPU perf.

(BTW if we were to use a mutable list, then please use ResizeArray)

[dsyme]

This looks OK

Re FlatList - yes, it's just alias for List. The belief once upon-a-time was that the pervasive use of linked lists (List) instead of flatter data structures (Array, ImmutableArray etc.) was hurting. So we added an alias FlatList for certain use of Lists where it felt there would be a perf gain for switching to a flat data structure. However when switching to a flat data structure we didn't see any real perf gains. However we left FlatList in just in case we wanted to try again. So it's still unclear if we should rip it out for simplicity, or try to resurrect the experiment. I'm OK with ripping it out at this stage I suppose since the experiment has been dormant so long.