Skip to content

Tweaks to Trivia::findNodeWhereRangeFitsIn #3169

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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?

@@ -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?

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