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

Add unit tests/performance tests. #867

Closed
7sharp9 opened this issue May 26, 2020 · 14 comments
Closed

Add unit tests/performance tests. #867

7sharp9 opened this issue May 26, 2020 · 14 comments
Assignees

Comments

@7sharp9
Copy link
Member

7sharp9 commented May 26, 2020

Im finding that formatting takes ~4 seconds for an 800 line file it would be great to know the areas that are slowing things down so this could be improved. Im finding myself not wanting to use fantomas as I know I have to wait every time I use it. Similar tools in Elm,Go or Rust are instantaneous and it would be great to know where to aim improvements in this library.

@nojaf
Copy link
Contributor

nojaf commented May 29, 2020

Hey @7sharp9, yes Fantomas isn't that fast when the files become larger.
I don't really have a lot of expertise in finding this kind of issues so I'm open for suggestions here.

@7sharp9
Copy link
Member Author

7sharp9 commented May 29, 2020

I was surprised the 800 lines took 4 seconds, most formatters don't have a perceivable latency. Benchmark.net is good for analysing but the functions that comprise fantomas would have to be tested in sections to see which parts are slow.

@jindraivanek
Copy link
Contributor

Biggest performance issue right now is Trivia collecting and querying. https://github.com/fsprojects/fantomas/blob/master/src/Fantomas/Trivia.fs

@7sharp9
Copy link
Member Author

7sharp9 commented May 29, 2020

    let triviaNodesFromAST =
        flattenNodeToList node
        |> filterNodes
        |> List.map mapNodeToTriviaNode
        |> List.choose id

The filter, map, choose can be combined to avoid extra iterations, as choose is the map+filter optimisation. Not sure if thats a slow bit though :-)

@jindraivanek
Copy link
Contributor

Slowest thing is probably that we use list to store TriviaNodes, and go through this list for every Triva query. Change it to something like dict<LineNumber, TriviaNode list> could be big perf win. Something more sophisticated like Segment tree may be better, but maybe it is overkill.

@7sharp9
Copy link
Member Author

7sharp9 commented May 29, 2020

First thing I guess is to get a baseline with benchmark.net

@nojaf
Copy link
Contributor

nojaf commented May 29, 2020

Ok, I'll try to set up a baseline with benchmark.net.

@nojaf nojaf self-assigned this May 29, 2020
@gerardtoconnor
Copy link

If performance is a concern, you should restructure using of list piping in all the private query functions and potentially use an optimised tree structure as suggested already. Using streaming source/sink pattern is more performant but cannot use compiler hack on list creation head first

@gerardtoconnor
Copy link

Profiling with Perfview will show where time is being spent, what is allocating heavily, just collect, run, stop and find process in profile export

@7sharp9
Copy link
Member Author

7sharp9 commented May 29, 2020

Isn't preview a windows tool? I mean, who uses windows?

@gerardtoconnor
Copy link

😂 I do, it was that or a MBP 😷 😜

@nojaf
Copy link
Contributor

nojaf commented Jun 3, 2020

@7sharp9 could you share that example file of 800 lines you talked about?
I'd like to have some more insights in your particular case.
Are we talking about regular formatting here or generating code based on AST nodes?

@nojaf
Copy link
Contributor

nojaf commented Jun 7, 2020

Just letting you know that we are on the right track here:
image

@nojaf
Copy link
Contributor

nojaf commented Sep 4, 2020

I'm going to close this as our benchmark test is in place and we can say that overall the performance has improved.
I'd be happy to follow-up on other concrete performance issues in a new issue.

@nojaf nojaf closed this as completed Sep 4, 2020
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

No branches or pull requests

4 participants