Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize simple range mappings: [for n in start..finish -> f n], &c. #16832

Merged
merged 13 commits into from
Mar 19, 2024

Conversation

brianrourkeboll
Copy link
Contributor

@brianrourkeboll brianrourkeboll commented Mar 7, 2024

Description

  • Apply the optimizations from Better integral range lowering: start..finish, start..step..finish #16650 to lists and arrays constructed from simple, direct range mappings, i.e. —

    • [for n in start..finish -> n]
    • [for n in start..finish -> f n]
    • [for n in start..step..finish -> n]
    • [for n in start..step..finish -> f n]
    • [|for n in start..finish -> n|]
    • [|for n in start..finish -> f n|]
    • [|for n in start..step..finish -> n|]
    • [|for n in start..step..finish -> f n|]

    — where f stands for any arbitrary transformation of n.

    This also applies to the syntax [for n in start..finish do yield n], etc., when there is exactly one yield, and to [for n in start..finish do f (); …; yield n], for any depth of sequential expressions, as long as there is still exactly one yield.

    The generated loop IL for [for n in start..finish -> n], etc., is identical to that generated for [start..finish].

Benchmarks

The performance improvement looks to be the same as that in #16650 (i.e., up to 8×), as would be expected, since it's the same count and initialization loop code. I can run a more thorough suite if desired.

| Categories                                                                      | Mean      | Ratio | Gen0    | Gen1    | Gen2    | Allocated  | Alloc Ratio |
|-------------------------------------------------------------------------------- |----------:|------:|--------:|--------:|--------:|-----------:|------------:|
| Array,[|for n in start..step..finish -> n * n|] (start=1,step=2,finish=65536)   | 205.05 us |  1.00 | 83.2520 | 83.2520 | 83.2520 |  384.49 KB |        1.00 |
| Array,[|for n in start..step..finish -> n * n|] (start=1,step=2,finish=65536)   |  43.95 us |  0.21 | 41.6260 | 41.6260 | 41.6260 |  128.04 KB |        0.33 |
|                                                                                 |           |       |         |         |         |            |             |
| List,[for n in start..step..finish -> n * n] (start=1,step=2,finish=65536)      | 197.15 us |  1.00 | 83.4961 | 71.7773 |       - | 1024.09 KB |        1.00 |
| List,[for n in start..step..finish -> n * n] (start=1,step=2,finish=65536)      | 158.13 us |  0.80 | 83.4961 | 71.5332 |       - |    1024 KB |        1.00 |

Checklist

  • Test cases added
  • Performance benchmarks added in case of performance changes
  • Release notes entry updated

Notes

This PR also optimizes simple mappings comprised of one or more sequential expressions like [for n in start..finish do printfn "abc"; yield n], where there is exactly one yield (5d52462). We could theoretically go even further and detect a statically-known number of yields > 1 and factor that back into the total iteration count, but that seems like a step too far for now.

Copy link
Contributor

github-actions bot commented Mar 7, 2024

❗ 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.300.md

@brianrourkeboll brianrourkeboll marked this pull request as ready for review March 8, 2024 17:05
@brianrourkeboll brianrourkeboll requested a review from a team as a code owner March 8, 2024 17:05
@T-Gro T-Gro enabled auto-merge (squash) March 18, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants