timholy / SnoopCompile.jl

Provide insights about latency (TTFX) for Julia packages
https://timholy.github.io/SnoopCompile.jl/dev/
Other
309 stars 48 forks source link

Add option to snoop commands to control spawning separate process vs eval'ing in existing process #252

Open NHDaly opened 3 years ago

NHDaly commented 3 years ago

Running snoop compilation in a separate process allows you to consistently measure complete compilation time on every run, without being affected by the state of the current process.

On the other hand, running the snoop compilation in the current process allows you to measure incremental compilation performance, which can also be useful in some contexts.

This PR attempts to add an option to all snoop commands to let you choose between those options. It also adds another function, @snoop_all, which runs @snoopi_deep, @snoopl and @snoopc together at the same time, so that you can compare results across them.


~Note that this branch is currently really messy, because it had a bunch of draft PRs merged into it, that have since been squash-merged into master, so we need to clean that up first as well.~ EDIT: Okay that's done. This branch currently only does @snoopi_deep and @snoopl, so we need to add @snoopc as well. And we need to add a pspawn-type flag to @snoopc. And if we want to allow running all of them in a separate process, we need to add it to @snoopi_deep as well.

This addresses this comment, for a request from @bkamins: https://github.com/timholy/SnoopCompile.jl/issues/251#issuecomment-886093390

codecov[bot] commented 3 years ago

Codecov Report

Merging #252 (c06d84e) into master (1abf0a2) will decrease coverage by 65.41%. The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #252       +/-   ##
===========================================
- Coverage   85.06%   19.65%   -65.42%     
===========================================
  Files           9        9               
  Lines        1587     1450      -137     
===========================================
- Hits         1350      285     -1065     
- Misses        237     1165      +928     
Impacted Files Coverage Δ
src/parcel_snoopl.jl 0.00% <0.00%> (-89.48%) :arrow_down:
src/parcel_snoopi_deep.jl 0.00% <0.00%> (-87.69%) :arrow_down:
src/invalidations.jl 0.00% <0.00%> (-86.52%) :arrow_down:
src/deep_demos.jl 0.00% <0.00%> (-57.90%) :arrow_down:
src/SnoopCompile.jl 66.66% <0.00%> (-33.34%) :arrow_down:
src/write.jl 89.47% <0.00%> (-1.84%) :arrow_down:
src/visualizations.jl 0.00% <0.00%> (ø)
src/parcel_snoopi.jl 94.11% <0.00%> (+0.28%) :arrow_up:
src/parcel_snoopc.jl 78.72% <0.00%> (+0.34%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1abf0a2...c06d84e. Read the comment docs.

bkamins commented 3 years ago

Thank you. Can you please ping me when this is ready to test? (or if you want me to review it now I can also try but I do not know the code base much)

NHDaly commented 3 years ago

So i think this is more or less ready to go, but for some reason that I can't explain, the first call to @snoopl is now failing with an IOError: write: broken pipe (EPIPE) during the serialize(). But only the first time, when i run it in the REPL, and then it seems to succeed consistently after that.

I have no idea why. In the last commit, 995ffd2, I tried simplifying the logic that's spawning the processes and passing the commands through, but it had no effect.

This is also why the tests are failing.

Any advice you have would be greatly appreciated!! 😬 🤞

=================

Otherwise, I've added tests and a docstring.

There's still no standalone tests for @snoopc and @snoopl with pspawn=false, and it would be nice to add those.

Finally, there's still no pspawn= option for @snoopi_deep or @snoop_all, which we could consider adding to make them more like the other functions. But this can be done in a follow-up PR.

NHDaly commented 3 years ago

Tl;dr: Tests failing and i don't know why - plz help!! 😬

NHDaly commented 3 years ago

@bkamins - you could try testing this now if you'd like

bkamins commented 3 years ago

Thank you. I do not get the error you report. However I have some general minor comments:

Thank you!

bkamins commented 3 years ago

Apart from that I have analyzed the results - and all works as I would expect. Superb work - thank you!

@pdeffebach - given the results of @snoop_all the difference between subset and select in compilation time is unfortunately due to the additional compilation that subset has (the extra logic it has). This is probably unavoidable (and this was expected).

NHDaly commented 3 years ago
  • running this macro swamps my terminal with the output; is this intended?

I've added an example with a ; at the end. Probably we should change the default show for the InferenceTimingNode returned as the first argument (tinf) from @snoopi_deep so that it doesn't print such a big tree, as well..

the two files you give .csv extension are not comma separated (it seems they are tab separated) Huh, until today I thought that you could have arbitrary delimiters, but it's still called a CSV. Should we rename them to .tsv? https://superuser.com/a/577912

could you please guide me what do the numbers in *snoopc.csv file mean (these in the beginning of each line)

Please see the updated docstring, with links to what to do with each of the results. You should be able to follow all of those separate guides for how to interpret the results. :)

bkamins commented 3 years ago

Thank you!

Huh, until today I thought that you could have arbitrary delimiters, but it's still called a CSV. Should we rename them to .tsv?

I would normally expect that tab delimited file has .txt extension. The .csv extension is not the end of the world though. The CSV.jl will read it correctly by auto-detection of a delimiter.

kimikage commented 5 months ago

This is not a technical issue, but the name all seems somewhat misleading. Perhaps this PR needs to be targeted and updated for SnoopCompile v3 or v2, but there is room for reconsideration of the name as well.