Skip to content

Conversation

@T-Gro
Copy link
Member

@T-Gro T-Gro commented Dec 8, 2022

This PR introduced a new fsc.exe switch --reportTimeToFile: to write collected activities to a .csv file.
This functionality is exposed in a way that other users of FCS can use too ( = not restricted to fsc.exe):

module Activity
      val addCsvFileListener: pathToFile: string -> IDisposable

To maintain a tabular structure of the .csv, only "well known" tag names are being exported.
This contains the main compilation phases (Parse > TypeCheck > ..) as well as gatherings for individual files.

As of now, the old functionality of "--times" remains intact (@vzarytovskii : Can we re-do what an existing flag does completely?), which means the code is still there.

If we could redo --times behavior (can be a separate PR for sure), we could:

  • Centralize 'CPU time' and 'GC collection' measurements to become an optional feature for any Activity
  • Have the console and .csv exporters working with the same data.
  • Add a filtering feature to enable smaller console output (? probably based on depth // distance from root activity? @safesparrow , @baronfel - if you have some use cases in mind on what would be sensible to filter on, please comment here)

The functionality can be exercised for a full build process on a project incl. it's dependencies like this:

dotnet msbuild `
-v:n `
-t:Rebuild `
-p:Configuration=Debug `
-p:FscToolPath="C:\code\fsharp\artifacts\bin\fsc\Debug\net7.0" `
-p:FscToolExe="fsc.exe"  `
-p:AdditionalFscCmdFlags="--times --reportTimeToFile:C:\tests\bigTestViaActivitySource.csv"

Example of a produced file for multiple projects at once:
bigTestViaActivitySource.csv

@T-Gro T-Gro requested a review from a team as a code owner December 8, 2022 15:50
@vzarytovskii
Copy link
Member

Yeah, I think it's fine to redo what the old flag does, as long as it will have the same "functionality" - will allow to easily look at what times are spent where.

@vzarytovskii
Copy link
Member

Can you please share an example of what does file look like?

@T-Gro
Copy link
Member Author

T-Gro commented Dec 8, 2022

Yeah, I think it's fine to redo what the old flag does, as long as it will have the same "functionality" - will allow to easily look at what times are spent where.

That's good. It is not mentioned as part of --help and web documentation only says "Displays timing information for compilation.", which would be maintained. OK

@T-Gro
Copy link
Member Author

T-Gro commented Dec 8, 2022

Can you please share an example of what does file look like?

Yep, uploaded.

@vzarytovskii
Copy link
Member

Also - curious, if we enable GC and memory collection for any/all activities, will there be any impact to times at all.
Theoretically, collecting gc info should be cheap if not free.

@T-Gro
Copy link
Member Author

T-Gro commented Dec 8, 2022

Also - curious, if we enable GC and memory collection for any/all activities, will there be any impact to times at all. Theoretically, collecting gc info should be cheap if not free.

I would like to enable via a special/function call and by default only use it for measurements where the current code already did it anyway.

Activity.start
vs (e.g., proposal)
Activity.startWithGcMeasurement

(and internally it would keep a well-known tag keyvaluepair with the GC data at start, then do the same at activity end, and print out the difference)

@T-Gro T-Gro requested review from KevinRansom, safesparrow and vzarytovskii and removed request for safesparrow December 13, 2022 11:44
0101
0101 previously approved these changes Dec 13, 2022
abonie
abonie previously approved these changes Dec 13, 2022
@T-Gro T-Gro dismissed stale reviews from abonie and 0101 via 721875a December 13, 2022 13:22
@T-Gro T-Gro merged commit 5e5344c into main Dec 14, 2022
@T-Gro T-Gro deleted the fsc-timing-to-file branch February 2, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

8 participants