Skip to content

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Jun 14, 2025

refs #3164 (comment)

  • Use Array.tryPick rather than Array.choose |> Array.tryHead
  • Use Option.orElseWith rather than 'Some betterChild -> Some betterChild'

Rather trivial in the grand scheme of things, but results of the Benchmarks project:

Before:

| Method | Mean     | Error   | StdDev   | Median   | Rank | Gen0     | Allocated |
|------- |---------:|--------:|---------:|---------:|-----:|---------:|----------:|
| Format | 174.5 ms | 8.05 ms | 23.22 ms | 164.4 ms |    1 | 333.3333 | 155.61 MB |

After:

| Method | Mean     | Error   | StdDev  | Rank | Gen0     | Allocated |
|------- |---------:|--------:|--------:|-----:|---------:|----------:|
| Format | 163.6 ms | 3.27 ms | 7.38 ms |    1 | 500.0000 | 155.17 MB |

My laptop is a bit variable with the runtime, but there' no real time change, just a small memory change (155.61MB to 155.24MB with the tryPick change, and to 155.17MB from orElseWith

- Use Array.tryPick rather than Array.choose |> Array.tryHead
- Use Option.orElseWith rather than 'Some betterChild -> Some betterChild'
match betterChildNode with
| Some betterChild -> Some betterChild
| None -> Some root
betterChildNode |> Option.orElseWith (fun () -> Some root)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that would perform better? Or this is more of a "I would have written it that way" thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Some betterChild -> Some betterChild appears to be allocatiing lots of extra instances of Option, which orElseWith avoids.
Maybe just doing Some _ -> betterChildNode would work as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appears to be allocatiing

Why are you getting that from? The before and after seem rather close?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Visual Studio allocation profiler shows this with the match:
image
and this with orElseWith
image
And BDN itself say it reduces allocations from 155.24MB to 155.17MB.
Admittedly this is only a small change in memory given the number of allocations

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, thanks for sharing!

@@ -158,12 +158,9 @@ let rec findNodeWhereRangeFitsIn (root: Node) (range: range) : Node option =
// The more specific the node fits the selection, the better
let betterChildNode =
root.Children
|> Array.choose (fun childNode -> findNodeWhereRangeFitsIn childNode range)
|> Array.tryHead
|> Array.tryPick (fun childNode -> findNodeWhereRangeFitsIn childNode range)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the real improvement, is it not?

Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@nojaf
Copy link
Contributor

nojaf commented Jun 27, 2025

Would you like a new release for this?
If so, please update the changelog.
Otherwise, we can leave this as is.

@Numpsy
Copy link
Contributor Author

Numpsy commented Jun 27, 2025

I don't think there's a need for a release just for this

@nojaf nojaf merged commit 51fade1 into fsprojects:main Jun 27, 2025
5 checks passed
@Numpsy Numpsy deleted the tweak_trivia branch June 29, 2025 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants