search

fsprojects / FSharp.Formatting

F# tools for generating documentation (Markdown processor and F# code formatter)
https://fsprojects.github.io/FSharp.Formatting/
Other
462 stars 155 forks source link

Minor Seq Optimizations #922

Closed 1eyewonder closed 1 week ago

1eyewonder commented 2 weeks ago

I was poking around the repo and saw these few spots where I thought we could get some minor performance gains. There are also quite a few areas where we have multiple Seq.Filter's next to each other. I would also like to suggest updating those unless maintainers know of a reason why we have multiple filter iterations next to each other. Hope this helps!

1eyewonder commented 2 weeks ago

Thanks for the quick review! I will go ahead and make these changes when I get back from travel early next week

nojaf commented 2 weeks ago

Thank you, safe travels!

1eyewonder commented 1 week ago

Here are the few areas I see consecutive Seq.filter/List.filter's used. Do you see any reason why we couldn't consolidate to help reduce iterations? The collections may be short enough where they were broken out for readability but I wanted to check to see if we wanted to include in this PR.

  1. https://github.com/fsprojects/FSharp.Formatting/blob/main/src/FSharp.Formatting.ApiDocs/GenerateModel.fs#L2424-L2425
  2. https://github.com/fsprojects/FSharp.Formatting/blob/main/src/FSharp.Formatting.ApiDocs/GenerateModel.fs#L2451-L2452
  3. https://github.com/fsprojects/FSharp.Formatting/blob/main/src/FSharp.Formatting.ApiDocs/GenerateModel.fs#L2464-L2466
  4. https://github.com/fsprojects/FSharp.Formatting/blob/main/src/FSharp.Formatting.ApiDocs/GenerateModel.fs#L2621-L2630
  5. https://github.com/fsprojects/FSharp.Formatting/blob/main/src/FSharp.Formatting.Common/YaafFSharpScripting.fs#L183-L184
  6. https://github.com/fsprojects/FSharp.Formatting/blob/main/src/FSharp.Formatting.Common/YaafFSharpScripting.fs#L284-L294
  7. https://github.com/fsprojects/FSharp.Formatting/blob/main/src/FSharp.Formatting.ApiDocs/Categorise.fs#L61-L81

Other than these questions, this PR is ready for review again 👍

nojaf commented 1 week ago

Here are the few areas I see consecutive Seq.filter/List.filter's used. Do you see any reason why we couldn't consolidate to help reduce iterations?

This codebase is rather old and has many authors. I don't believe there is a good reason these cases are not combined. Could you address these as well?