[CompilerPerf] Fix large ranges

[dsyme]

The F# compiler has historically used a 32-bit integer for "position" information and a 64-bit integer for "range" information. But this means we lose information and can only accurately handle:

• file references up to 16384 referenced files (across all projects being analysed by FSharp.Compiler.Service)
• line length up to 512
• file length up to 65536
• intra-file range spans up to 32768

So we have bugs where long lines and/or long files and/or lots-of-files give inaccurate information because we compress the range information into a 64-bit integer.

This PR is a possible fix for this by using 128-bits. This gives limits:

• file references up to ~16million referenced files (across all projects being analysed by FSharp.Compiler.Service)
• line length up to ~1million
• file length up to ~2billion

However I’m concerned about perf implications of using 128bits for range values since these get passed around everywhere. I will test perf on this PR and will compare with other compilers such as Roslyn.

(Quote of the day: "F# file lengths of 640K should be enough for anyone!")

[KevinRansom]

@dsyme, conflicts to resolve. How do you propose investigating the perf implications you mention?

[KevinRansom]

Lots of bit twiddling and masking here. Looks good though.

[dsyme]

@dsyme, conflicts to resolve. How do you propose investigating the perf implications you mention?

@KevinRansom Good question. I ran the compiler perf script a few times and it seems to be slower by around 3-5%, which is kind of what I expected. BUT, I'm always a bit sceptical of the reliability of the data collection in that compiler perf script....

It's possible we should just do this properly by changing to use the character-offset-from-start-of-file. That is, however, a more intrusive change....

[auduchinok]

It's possible we should just do this properly by changing to use the character-offset-from-start-of-file. That is, however, a more intrusive change....

That would be great.

[abelbraaksma]

Great work! I don't think you should let the 3% performance drop stop you from merging this, it solves quite a few stability problems already. And for code-generation this is great, because sometimes these files get much larger than 64k lines.

This would solve some really old issues, for instance with source stepping, i.e. #758 from 2015 (I know there were others, but couldn't locate them anymore).

It also has a StackOverflow reference: https://stackoverflow.com/questions/33794570.

[abelbraaksma]

In case it helps, there's a repro sln in #758 that could perhaps be used to write a test case for this.

[cartermp]

@dsyme I'd like to consider this for dev16, as the issue in #758 implies this is the right stability fix, and will also help and other tooling that do things that aren't AST-based

[dsyme]

@dsyme I'd like to consider this for dev16, as the issue in #758 implies this is the right stability fix, and will also help and other tooling that do things that aren't AST-based

@cartermp ok

[KevinRansom]

@dsyme, sorry mate resolution required :-(

[KevinRansom]

@dsyme merge issues :-)

[cartermp]

After talking with @dsyme about this, the approach here is fundamentally flawed, and packing in more information here will result in compiler and tooling slowdowns that we're hesitant to accept. The right way to store this information is as an offset, calculating range information as needed rather than flowing that larger amount of data everywhere. Such a change is more invasive, but the correct approach.

@dsyme do you want to keep this open?

[dsyme]

After talking with @dsyme about this, the approach here is fundamentally flawed, and packing in more information here will result in compiler and tooling slowdowns that we're hesitant to accept.

I do think the slight perf slow down is probably still acceptable given we just have to fix this problem. I'll bring this up to date and do some more perf checks.

[dsyme]

This is ready, I think we should merge it and just be done with the issue. We have to be correct in these cases and the indications above are that performance is reasonable.

[cartermp]

@KevinRansom @TIHan could you give this a review? Given @dsyme's investigations I think this is worth taking, especially because it fixes at least two bugs. It'd be good to do it the Right Way ™️, but that's very invasive work and this is ready to be flighted in dev16 previews.

[TIHan]

Created an issue to track that we need perf improvements in the future: #5826