CheckFormatStrings.parseFormatStringInternal appears to split source files many times over causing LOH allocations

[davkean]

Trace: https://developercommunity.visualstudio.com/content/problem/379694/another-repro-trace-for-github-bug-4718.html
Trace: https://developercommunity.visualstudio.com/content/problem/245786/slow-f-editing-experience-up-to-a-minute-until-typ.html

GC Rollup By Generation

Gen	Count	MaxPause	MaxPeak MB	Max AllocMB/sec	TotalPause	TotalAlloc MB	Alloc MB/MSec GC	Survived MB/MSec GC	MeanPause	Induced
ALL	506	953.4	2,638.9	116,179.562	18,629.5	231.4	0.0	34.716	36.8	0
0	443	12.7	2,638.9	116,179.562	2,303.8	93.8	10.0	0.037	5.2	0
1	43	13.2	2,636.8	96.406	265.3	-1.6	1.1	0.228	6.2	0
2	20	953.4	2,638.9	1.985	16,060.4	139.1	69.4	677.189	803.0	0

In the above trace, we noticed that we had a tonne of Gen2 collections due to LOH allocations. The first one we noticed that parseFormatStringInternal splits (what appears to be) the source file many times over. For large source files these all end up on the LOH heap:

[davkean]

It also does a Replace that is causing a bunch too:

[forki]

mmm that's interesting. I just looked briefly at that file/function and there is no direct invoction of Split nor Replace. Do you have more details about that stack trace?

[AviAvni]

@forki look at the code in branch dev15.9 https://github.com/Microsoft/visualfsharp/blob/dev15.9/src/fsharp/CheckFormatStrings.fs

[forki]

@AviAvni ? so you are saying it's already fixed? or what?

[davkean]

Possibly in #5121 - but that was 5 months ago, as to why that hasn't made it in 15.9...

[AviAvni]

@forki not really the code moved to somewhere else in #5121
to https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/NameResolution.fs#L1538-L1548
but i have a code that can fixed that

seq {
        let mutable pos = 0
        yield pos
        for c in source do
            if c = '\r' then ()
            if c = '\n' then yield pos
            else pos <- pos + 1
    }
    |> Seq.toArray

I'll send PR soon

[forki]

Ouch. that part looks indeed dangerous

[AviAvni]

@forki when checking the difference with my code

open System.IO

// Learn more about F# at http://fsharp.org
// See the 'F# Tutorial' project for more help.

let positions1 (source:string) =
    let source = source.Replace("\r\n", "\n").Replace("\r", "\n")
    source.Split('\n')
    |> Seq.map (fun s -> String.length s + 1)
    |> Seq.scan (+) 0
    |> Seq.toArray

let positions2 (source:string) =
    seq {
        let mutable pos = 0
        yield pos
        for c in source do
            if c = '\r' then ()
            if c = '\n' then yield pos
            else pos <- pos + 1
    }
    |> Seq.toArray

let str = File.ReadAllText(@"C:\israel-public-transportation\trips.txt")

#time

let v1 = positions1 str
let v2 = positions2 str

you can see the result that no gc on really big strings the file is 28MB

> 
Real: 00:00:00.238, CPU: 00:00:00.265, GC gen0: 11, gen1: 5, gen2: 1
val v1 : int [] =
  [|0; 64; 113; 162; 211; 260; 309; 358; 407; 456; 505; 554; 603; 652; 701;
    750; 799; 848; 897; 946; 995; 1044; 1093; 1142; 1191; 1240; 1289; 1338;
    1387; 1436; 1485; 1534; 1583; 1632; 1681; 1730; 1779; 1828; 1877; 1926;
    1975; 2024; 2073; 2122; 2171; 2220; 2271; 2322; 2373; 2424; 2475; 2526;
    2577; 2628; 2679; 2730; 2781; 2832; 2883; 2934; 2985; 3036; 3087; 3138;
    3189; 3240; 3291; 3342; 3393; 3444; 3495; 3546; 3597; 3648; 3699; 3750;
    3801; 3850; 3899; 3948; 3997; 4046; 4095; 4144; 4193; 4242; 4291; 4340;
    4389; 4438; 4487; 4536; 4585; 4634; 4683; 4732; 4781; 4830; 4879; 4928;
    ...|]

> 
Real: 00:00:00.155, CPU: 00:00:00.156, GC gen0: 0, gen1: 0, gen2: 0
val v2 : int [] =
  [|0; 64; 113; 162; 211; 260; 309; 358; 407; 456; 505; 554; 603; 652; 701;
    750; 799; 848; 897; 946; 995; 1044; 1093; 1142; 1191; 1240; 1289; 1338;
    1387; 1436; 1485; 1534; 1583; 1632; 1681; 1730; 1779; 1828; 1877; 1926;
    1975; 2024; 2073; 2122; 2171; 2220; 2271; 2322; 2373; 2424; 2475; 2526;
    2577; 2628; 2679; 2730; 2781; 2832; 2883; 2934; 2985; 3036; 3087; 3138;
    3189; 3240; 3291; 3342; 3393; 3444; 3495; 3546; 3597; 3648; 3699; 3750;
    3801; 3850; 3899; 3948; 3997; 4046; 4095; 4144; 4193; 4242; 4291; 4340;
    4389; 4438; 4487; 4536; 4585; 4634; 4683; 4732; 4781; 4830; 4879; 4928;
    ...|]

[forki]

iz's still creating a full copy. Do we really need that?

[AviAvni]

What full copy?

[forki]

a full copy of the source file in memory.
In parseFormatStringInternal we are just iterating over that thing anyway can't we just somehow handle the line breaks on the fly while iterating over the string?

[AviAvni]

OK I debugged it. I only able to reproduce calling to this code when visual studio F# tools(so able to do it only from the dev15.9 branch)
the using of the source is because of this call
https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/service/service.fs#L1734
that specify TcResultsSink.WithSink which I don't understand what is this beside this parsing
anyway after this this call this method called again without the source
So I can send a PR that remove all of this allocations but don't know why this source is used so don't know what to to test if I change it