Adds naive parallel formatting implementation #2717

TheAngryByrd · 2023-01-14T20:52:52Z

Fix for #2624. Does a rather naive "collect all the async and run at the end" but seems to give really promising numbers.

Test results on my machine

BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22000.1455/21H2)
12th Gen Intel Core i9-12900F, 1 CPU, 24 logical and 16 physical cores
.NET SDK=7.0.100
[Host] : .NET 7.0.0 (7.0.22.51805), X64 RyuJIT AVX2 DEBUG
Job-XAWGZF : .NET 7.0.0 (7.0.22.51805), X64 RyuJIT AVX2

Server=True IterationCount=1 RunStrategy=ColdStart

Method	projects	Mean	Error	StdDev	Median	Ratio	RatioSD	Rank	Gen0	Gen1	Gen2	Allocated	Alloc Ratio
FormatCode_Sequentially	FAKE	3,872.3 ms	515.20 ms	593.3 ms	3,731.8 ms	1.00	0.00	2	16000.0000	10000.0000	1000.0000	4.55 GB	1.00
FormatCode_Parallel	FAKE	753.8 ms	371.76 ms	428.1 ms	602.2 ms	0.19	0.06	1	4000.0000	2000.0000	-	4.55 GB	1.00

FormatCode_Sequentially	FsToo(...)dling [23]	862.6 ms	230.28 ms	265.2 ms	792.4 ms	1.00	0.00	2	3000.0000	2000.0000	-	1.15 GB	1.00
FormatCode_Parallel	FsToo(...)dling [23]	193.2 ms	97.44 ms	112.2 ms	164.2 ms	0.22	0.03	1	3000.0000	2000.0000	-	1.15 GB	1.00

FormatCode_Sequentially	Plotly.NET	1,965.1 ms	300.60 ms	346.2 ms	1,884.8 ms	1.00	0.00	2	6000.0000	3000.0000	-	2.29 GB	1.00
FormatCode_Parallel	Plotly.NET	445.3 ms	142.50 ms	164.1 ms	392.7 ms	0.23	0.07	1	12000.0000	3000.0000	1000.0000	2.3 GB	1.00

FormatCode_Sequentially	fsharp	6,183.1 ms	634.73 ms	731.0 ms	6,004.4 ms	1.00	0.00	2	20000.0000	10000.0000	2000.0000	9.43 GB	1.00
FormatCode_Parallel	fsharp	2,046.6 ms	587.11 ms	676.1 ms	1,807.8 ms	0.33	0.09	1	15000.0000	6000.0000	2000.0000	9.44 GB	1.00

Please verify your pull request is respecting our Pull request ground rules.
You can find more information in our Contributors documentation section.

src/Fantomas.CLI.Benchmarks/Runners.fs

src/Fantomas/Program.fs

CHANGELOG.md

src/Fantomas/Fantomas.fsproj

src/Fantomas/Program.fs

src/Fantomas.CLI.Benchmarks/Runners.fs

nojaf · 2023-01-24T09:55:34Z

@TheAngryByrd is the --check flag also running in parallel?

TheAngryByrd · 2023-01-24T15:03:47Z

@TheAngryByrd is the --check flag also running in parallel?

I didn't touch it so doubtful.

TheAngryByrd · 2023-01-29T18:39:47Z

@TheAngryByrd is the --check flag also running in parallel?

I didn't touch it so doubtful.

Seems to already be running parallel: https://github.com/fsprojects/fantomas/blob/main/src/Fantomas/Format.fs#L118

TheAngryByrd

1 requirement needed to be redefined but ready to review otherwise.

TheAngryByrd · 2023-01-29T19:19:43Z

src/Fantomas.Tests/Integration/ConfigTests.fs

@@ -52,7 +52,7 @@ end_of_line=cr
 """
        )

-    let args = sprintf "%s %s" NormalVerbosity fileFixture.Filename
+    let args = sprintf "%s %s" DetailedVerbosity fileFixture.Filename


We discussed we didn't want to show all exception messages unless they came from specific fantomas exceptions. However, fantomas uses failwith in many places instead of it's own exceptions. Should we keep this requirement, always show all the exceptions, or rework all the places with failwith to throw a known Fantomas exception?

We can focus on FormatException for now. Show that specific message for normal logging.
And print something generic in case it came for failwith.
Unless we are in verbose mode, then I think we show the full exception.

nojaf

Great work Jimmy! This looks really good!

As always, I had some nitpicks but with some tweaks, we can merge this first version I think.
I think we will need to dogfood this a bit to ensure everything is what we want.

nojaf · 2023-01-30T14:38:42Z

src/Fantomas.Benchmarks/CLI.Runners.fs

+        Program.main args |> ignore
+
+    // Currently have to manually pull these repositories
+    [<Params("FsToolkit.ErrorHandling", "FAKE", "fsharp", "Plotly.NET")>]


I think we can re-use some work of regression.fsx perhaps.

match Array.tryLast fsi.CommandLineArgs with | Some "--benchmark" -> printfn "do benchmark stuff" | _ -> // Ensure Fantomas dll is built (wrap fantomasRoot "dotnet" $"build -c Release {fantomasProject}").Wait() // Run repositories for repo in repositories do (run repo).Wait()

and refactor configurtion.fsx to create a second list with projects we want to use for this benchmark.

nojaf · 2023-01-30T14:40:52Z

src/Fantomas.Benchmarks/CLI.Runners.fs

+        printfn $"git reset --hard  {projectDir}"
+
+        let psi =
+            System.Diagnostics.ProcessStartInfo("git", "reset --hard", WorkingDirectory = projectDir)


I would replace this with CliWrap and just call the regression.fsx script with the additional argument.
Cli.Wrap("dotnet").WithWorkingDirectory("the correct pwd").WithArguments(@"fsi regressions.fsx --benchmark")

nojaf · 2023-01-30T14:41:13Z

src/Fantomas.Benchmarks/Program.fs


 [<EntryPoint>]
 let main _ =
+
+    // BenchmarkRunner.Run<ParallelVsSequentialFormatting>() |> ignore


Nit: remove old comment.

nojaf · 2023-01-30T14:45:34Z

src/Fantomas.Tests/Integration/ConfigTests.fs

@@ -52,7 +52,7 @@ end_of_line=cr
 """
        )

-    let args = sprintf "%s %s" NormalVerbosity fileFixture.Filename
+    let args = sprintf "%s %s" DetailedVerbosity fileFixture.Filename


We can focus on FormatException for now. Show that specific message for normal logging.
And print something generic in case it came for failwith.
Unless we are in verbose mode, then I think we show the full exception.

nojaf · 2023-01-30T14:48:51Z

src/Fantomas/Program.fs

@@ -17,6 +19,7 @@ type Arguments =
    | [<Unique>] Out of string
    | [<Unique>] Check
    | [<Unique>] Daemon
+    | [<Unique>] Sequential


Shower thought: do we even need this setting? As we don't have it for --check right now.
Less is more right? Thoughts @TheAngryByrd @dawedawe @josh-degraw?

I'd prefer to go without it.

nojaf · 2023-01-30T14:52:16Z

src/Fantomas/Program.fs

+                | VerbosityLevel.Normal ->
+                    match ex with
+                    | :? Fantomas.Core.FormatException as fe -> fe.Message
+                    | :? CodeFormatException as fe -> fe.Message


I don't think CodeFormatException is raised anywhere?

nojaf · 2023-02-03T11:11:24Z

Hi all, I made some liberal changes to this PR:

Normal logging:

Verbose logging:

Cliffnotes:

Use -v for --verbosity instead of version`.
Raise a specific ParseException.
Set the Serilog level based on our internal flag.
Removed the Sequential flag.
Don't show INF when you are in Normal mode. MSBuild also doesn't show any specific level in their logs.

This by no means needs to be the final proposal for v6, but I think it is a step in the good direction and allows us to merge @TheAngryByrd's work.

What do you think @dawedawe @TheAngryByrd @josh-degraw @edgarfgp?

dawedawe · 2023-02-03T13:31:15Z

I like it a lot. Had something similar in mind but planned to wait for the merge of this one.

TheAngryByrd force-pushed the 2624-parallel-formmatting branch 6 times, most recently from 6526461 to 757a409 Compare January 14, 2023 21:28

TheAngryByrd commented Jan 14, 2023

View reviewed changes

src/Fantomas.CLI.Benchmarks/Runners.fs Outdated Show resolved Hide resolved

src/Fantomas/Program.fs Show resolved Hide resolved

src/Fantomas/Program.fs Outdated Show resolved Hide resolved

src/Fantomas/Program.fs Outdated Show resolved Hide resolved

nojaf reviewed Jan 15, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

nojaf reviewed Jan 15, 2023

View reviewed changes

src/Fantomas/Fantomas.fsproj Outdated Show resolved Hide resolved

nojaf reviewed Jan 15, 2023

View reviewed changes

src/Fantomas/Program.fs Outdated Show resolved Hide resolved

TheAngryByrd force-pushed the 2624-parallel-formmatting branch from 04597c4 to 3967e67 Compare January 15, 2023 22:43

TheAngryByrd changed the base branch from main to v6.0 January 15, 2023 22:46

TheAngryByrd commented Jan 15, 2023

View reviewed changes

src/Fantomas/Program.fs Outdated Show resolved Hide resolved

src/Fantomas.CLI.Benchmarks/Runners.fs Outdated Show resolved Hide resolved

TheAngryByrd force-pushed the 2624-parallel-formmatting branch from 3967e67 to 248d834 Compare January 15, 2023 22:53

nojaf force-pushed the v6.0 branch 2 times, most recently from c0771e5 to 219f34b Compare January 20, 2023 10:28

nojaf force-pushed the v6.0 branch from 9ffd88e to 33f45da Compare January 24, 2023 12:57

safesparrow mentioned this pull request Jan 25, 2023

[Feature request] Less verbose output #2627

Closed

6 tasks

TheAngryByrd force-pushed the 2624-parallel-formmatting branch from 4de0b58 to 2f5e9d4 Compare January 29, 2023 19:07

TheAngryByrd marked this pull request as ready for review January 29, 2023 19:08

TheAngryByrd commented Jan 29, 2023

View reviewed changes

nojaf requested changes Jan 30, 2023

View reviewed changes

nojaf mentioned this pull request Jan 30, 2023

Revisit --profile flag #2751

Closed

nojaf force-pushed the v6.0 branch from 7ca19ab to 3d0fe7c Compare February 3, 2023 10:08

TheAngryByrd added 3 commits February 3, 2023 11:11

Adds naive parallel formatting implementation

38d5e79

Refactoring FantomasCliBenchmarks

7f37a4b

Removes unnecessary servergc in fsproj

a09bdbd

TheAngryByrd added 8 commits February 3, 2023 11:11

Changes parallel to sequential, adds end report

e62a4b7

Refactorings

96b5698

Show error messages for Fantomas exceptions

96f962c

renaming

205a4ed

better benchmarking report for seq vs parallel

89a57bc

Tests passing

5e3605c

Consilodate benchmarks to single project

52b5a16

Fixing rebase issues

680636e

nojaf force-pushed the 2624-parallel-formmatting branch from 72d5915 to 680636e Compare February 3, 2023 10:11

nojaf added 2 commits February 3, 2023 11:21

Remove sequential flag.

1e48c94

Tweak logging.

5029ee8

nojaf approved these changes Feb 4, 2023

View reviewed changes

nojaf merged commit 2521d0d into fsprojects:v6.0 Feb 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds naive parallel formatting implementation #2717

Adds naive parallel formatting implementation #2717

TheAngryByrd commented Jan 14, 2023 •

edited

Loading

nojaf commented Jan 24, 2023

TheAngryByrd commented Jan 24, 2023

TheAngryByrd commented Jan 29, 2023

TheAngryByrd left a comment

TheAngryByrd Jan 29, 2023

nojaf Jan 30, 2023

nojaf left a comment

nojaf Jan 30, 2023

nojaf Jan 30, 2023

nojaf Jan 30, 2023

nojaf Jan 30, 2023

nojaf Jan 30, 2023

dawedawe Jan 30, 2023

nojaf Jan 30, 2023

nojaf commented Feb 3, 2023

dawedawe commented Feb 3, 2023

Adds naive parallel formatting implementation #2717

Adds naive parallel formatting implementation #2717

Conversation

TheAngryByrd commented Jan 14, 2023 • edited Loading

nojaf commented Jan 24, 2023

TheAngryByrd commented Jan 24, 2023

TheAngryByrd commented Jan 29, 2023

TheAngryByrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf commented Feb 3, 2023

dawedawe commented Feb 3, 2023

TheAngryByrd commented Jan 14, 2023 •

edited

Loading