Skip to content

Use FSharp.Core.Extended #3164

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

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

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented May 19, 2025

Before

| Method | Mean     | Error    | StdDev   | Rank | Gen0     | Gen1     | Allocated |
|------- |---------:|---------:|---------:|-----:|---------:|---------:|----------:|
| Format | 60.39 ms | 1.102 ms | 1.312 ms |    1 | 888.8889 | 333.3333 | 155.13 MB |

After

| Method | Mean     | Error    | StdDev   | Rank | Gen0     | Gen1     | Allocated |
|------- |---------:|---------:|---------:|-----:|---------:|---------:|----------:|
| Format | 60.51 ms | 0.489 ms | 0.458 ms |    1 | 888.8889 | 333.3333 | 155.13 MB |

It didn't really do anything for this repo.
Maybe I missed something in setting this up, although we probably don't use any of the optimized APIs.

//cc @vzarytovskii

@vzarytovskii
Copy link

Yeah, fair. I will look at the fantomas codebase and see what it can benefit from.

@Numpsy
Copy link

Numpsy commented May 23, 2025

Does it need a open FSharp.Core.Extended.Collections to use the alternate functions?

Yeah, fair. I will look at the fantomas codebase and see what it can benefit from.

A quick attempt at profiling suggests that a version of Seq.tryHead using ValueOption might shave off some Option allocations in a few places fwiw

@vzarytovskii
Copy link

Does it need a open FSharp.Core.Extended.Collections to use the alternate functions?

Yeah, fair. I will look at the fantomas codebase and see what it can benefit from.

A quick attempt at profiling suggests that a version of Seq.tryHead using ValueOption might shave off some Option allocations in a few places fwiw

That's on my list, however on latest runtime, reference types which do not escape the stack frame can be lifted to be allocated on stack. So this might be an improvement even without that change.

@Numpsy
Copy link

Numpsy commented May 24, 2025

Does the automatic stack allocation require tryHead to be inline to remove the allocations in .NET 9?

A small obervation when I ran the VS memory profiler on the 'Format' benchmark in case it's of interest - it says there are 67781 allocations of 0 length arrays of TriviaNode -

image

Those all appear to be from creating Queues with an explicit 0 capacity at

let nodesBefore = Queue<TriviaNode>(0)
because it appears that Queue always creates a new array when a capacity is specified, even if the capacity is 0, and just using the default constructor instead there reduces the reported memory allocations in the benchmark from 155.09 MB to 153.53 MB

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.

3 participants