Range.mkRange is allocating large amount of data normalizing strings

[davkean]

Trace: https://developercommunity.visualstudio.com/content/problem/379653/vs-slow-typing-and-hang-1.html.

It looks like Range.mkRange is allocating 16 MB of data just normalizing a path via Path.GetFullPath. I have no idea what this code does, or under what circumstances this is hit - but we should try avoid normalizing files over and over again.

[baronfel]

Are you in debug mode here? Some quick possibilities:
a) could be related to the size of the fileIndexTable global state at https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/range.fs#L136, or
b) could be related to the DebugCode member calculation at https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/range.fs#L177, especially since that member reads all lines of the files, does a lazy skip/take over it, string concats the lines (not sure if that uses stringbuffer) and then does a substring. That work is part of the debuggerdisplay for this class too, so could be quite painful.

[davkean]

Above is taken from a real world trace, I'm just interpreting the trace.

[cartermp]

This function is called a lot, but addressing this may be more complicated than finding a way to avoid calling GetFullPath. We shouldn't be carrying around this file data with ranges at all, which implies we need to redesign this data structure and where it is used, which won't be easy.

[cartermp]

On second thought, this function is just plain wrong. If the path is rooted and we fail to get the full path, we still just return the given path ... surely that cannot be correct, assuming the goal is to normalize a path?

[dsyme]

I haven't tested it but this looks like a possible conservative fix to the allocation problem:

https://github.com/Microsoft/visualfsharp/compare/master...dsyme:f79?expand=1

Basically we store multiple keys for non-canonicalized file paths, each with the same numeric entry.

[dsyme]

On second thought, this function is just plain wrong. If the path is rooted and we fail to get the full path, we still just return the given path ... surely that cannot be correct, assuming the goal is to normalize a path?

I think the original code is correct enough, so

c:\foo\..\abc.fs --> gets stored under c:\abc.fs (so we always show that for error reports)
a\..\abc.fs --> gets stored under a\..\abc.fs
some-path-with-illegal-characters --> gets stored under some-path-with-illegal-characters

The second case is imperfect, but it doesn't really matter, and n any case VS and msbuild gives paths to the compiler fully qualified so it doesn't matter

The third case is just protection against dodgy file names (they are caught elsewhere in any case)

[cartermp]

@dsyme The diff looks reasonable. I can put together a perf test to see the difference. Can you submit that as a PR?

[dsyme]

https://github.com/Microsoft/visualfsharp/compare/master...dsyme:f79?expand=1

[cartermp]

The call is now only happening if we haven't already indexed the path, which should alleviate the allocations here. Further work to remove these allocations likely would be as a redesign of ranges, which is a far bigger problem.

#5826 will track this.