Computed collections: optimize simple mappings with preludes

[brianrourkeboll]

Description

• Addresses Speed up for … in xs -> … in computed collections #16948 (comment).

  That is, before this, when a for … in xs -> … expression was the last expression in the comprehension, but one or more let-bindings or sequential expressions preceded it, the simple mapping (Speed up for … in xs -> … in computed collections #16948) and integral range (Optimize simple range mappings: [for n in start..finish -> f n], &c. #16832) optimizations would not be applied. With this PR, they are.

  Source code like

  [lety= f ()inletz= g ()infor … in … -> …]

  and

  [f (); g ();for … in … -> …]

  is now transformed (in an intermediate step) into something like

  lety= f ()inletz= g ()in[for … in … -> …]

  and

  f (); g ();[for … in … -> …]

  to allow the existing pattern-matching to succeed.

• Also updates Speed up for … in xs -> … in computed collections #16948 and directly emits fast loops instead of calling Array.map/List.map.

Checklist

• Test cases added.
• Release notes entry updated.

Note to reviewers

When looking at the baselines, it is useful to compare each function's IL with its source, paying specific attention to the order in which things happen: e.g., if two functions f and g are passed in and invoked in that order, ensure that you see the ldarg.0 instruction (for f) before the ldarg.1 instruction (for g), etc. If these happen outside of the for-loop in the source, likewise ensure that they do so in the IL: e.g., if f and g are called before the loop in the source, and if the loop ends in blt.un.s IL_0022 in the IL, ensure that f and g are loaded and called before IL_0022 (or before the call to map, depending).

(I have looked, and it looks right to me.)

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/8.0.400.md

[T-Gro]

Did you manage to VS-breakpoint capabilities and looking into the values, such as seeing the value of z after let z = g() in your samples?

[brianrourkeboll]

@T-Gro

Did you manage to VS-breakpoint capabilities and looking into the values, such as seeing the value of z after let z = g() in your samples?

Thanks for the reminder, I had been meaning to do that. I'll add something to TheBigFileOfDebugStepping.fsx and make sure it behaves as expected.

[brianrourkeboll]

@T-Gro

Did you manage to VS-breakpoint capabilities and looking into the values, such as seeing the value of z after let z = g() in your samples?

This particular part is fine, but it seems like the non-inlined call to map from #16948 causes the debug points in the mapping body not to be hit.

Maybe I'll just go ahead and emit loops directly for that instead. Since that will affect more baselines, would you rather I did that in a separate PR?

[T-Gro]

@T-Gro

Did you manage to VS-breakpoint capabilities and looking into the values, such as seeing the value of z after let z = g() in your samples?

This particular part is fine, but it seems like the non-inlined call to map from #16948 causes the debug points in the mapping body not to be hit.

Maybe I'll just go ahead and emit loops directly for that instead. Since that will affect more baselines, would you rather I did that in a separate PR?

That is fine, diff in baselines is expected.
Please, do it here in this PR.

[psfinaki]

Hey Brian, this looks promising - do you have any benchmarks for this also?

[brianrourkeboll]

Hmm @T-Gro, it seems like the debugging experience for for-loops and computed collection expressions is already pretty inconsistent, which makes it tough to be sure that I'm not regressing anything here.

These reproduce for me if I compile an fsx file using the following command (and in a console app) using the shipped 8.0.204 SDK:

dotnet "C:\Program Files\dotnet\sdk\8.0.204\FSharp\fsc.dll"--debug+--tailcalls---optimize- .\tests\walkthroughs\DebugStepping\Comparison.fsx

For example, stepping through a simple, top-level for-loop already behaves differently depending on whether the thing being enumerated over is a range, a sequence, an array, or a list:

bare_loop_debug_comparison.mp4

There are also some scenarios where breakpoints already cannot be set in computed collections, depending on the combination of comprehension type (list, array, seq), enumerable type (list, array, seq, range), simple body versus sequential expression body, explicit yield versus implicit yield, etc.:

unsettable_breakpoints_2.mp4

I think the best I can do is try to keep the experience with this update as close as possible to the status quo. I'll let you know when I think it's close enough.

[brianrourkeboll]

@psfinaki

do you have any benchmarks for this also?

I reran the benchmarks from #16948, and the results were the same within the margin of error. Those benchmarks were just testing an identity mapping; I'm sure it's possible to come up with a benchmark where you could notice the difference between the inlined loop and the closure invocation in List.map/Array.map, but I think the difference between those two is going to be dwarfed by the difference between either of them and the original IEnumerable<_>/ArrayCollector-based implementation anyway.

[T-Gro]

I think the best I can do is try to keep the experience with this update as close as possible to the status quo. I'll let you know when I think it's close enough.

Thanks for looking into this and checking it Brian. Keeping the status quo working is a good goal :-).

[psfinaki]

@psfinaki

do you have any benchmarks for this also?

I reran the benchmarks from #16948, and the results were the same within the margin of error. Those benchmarks were just testing an identity mapping; I'm sure it's possible to come up with a benchmark where you could notice the difference between the inlined loop and the closure invocation in List.map/Array.map, but I think the difference between those two is going to be dwarfed by the difference between either of them and the original IEnumerable<_>/ArrayCollector-based implementation anyway.

Alright, yeah that makes sense.

[brianrourkeboll]

FYI, I probably won't have a chance to finish this up till next week.

[brianrourkeboll]

@T-Gro@psfinaki I believe this is ready now.

[psfinaki]

LGTM. Thanks for restoring the walkthrough also.
Good job 🚀