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

Small memory and performance optimization: Less nested seq-wrapping on recursive functions #1511

Merged
merged 5 commits into from
Jul 8, 2024

Conversation

Thorium
Copy link
Member

@Thorium Thorium commented Jul 5, 2024

Small memory and performance optimization:

  • Less nested seq-wrapping on recursive functions

As a side note:
There are some cases where FSharp.Data does some manual recursive string-parsing-functions. More could be done on those: If we'd accept a new dependency to System.Memory on .NET Standard 2.0 version, we'd get .AsSpan and ReadOnlySpan<char> which could be used to gain speed and save memory (especially on .NET Standard 2.1 where those are available out-of-the-box and System.Memory reference is not needed).

@cartermp
Copy link
Collaborator

cartermp commented Jul 7, 2024

If you run dotnet fake build -t Format then it should probably pass CI

@Thorium
Copy link
Member Author

Thorium commented Jul 7, 2024

Thanks, done, I didn't know Fantomas had infected this repo as well.

@Thorium
Copy link
Member Author

Thorium commented Jul 7, 2024

I'd also like to get this change for FSharp.Data now that it's F# 6:
fsprojects/FSharp.TypeProviders.SDK#407

@cartermp
Copy link
Collaborator

cartermp commented Jul 7, 2024

Mmm, this change causes a stack overflow when generating docs. Could you investigate? https://github.com/fsprojects/FSharp.Data/actions/runs/9828724527/job/27132994427?pr=1511#step:6:1748

@Thorium
Copy link
Member Author

Thorium commented Jul 7, 2024

Ok, fixed. The docs were actually nice edge-case unit-tests. :-)

@cartermp cartermp merged commit 71c95c7 into fsprojects:main Jul 8, 2024
2 checks passed
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