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

NHDaly · 2021-07-25T17:23:40Z

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: #251 (comment)

codecov · 2021-07-25T17:33:27Z

Codecov Report

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

@@             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%)`	⬇️
src/parcel_snoopi_deep.jl	`0.00% <0.00%> (-87.69%)`	⬇️
src/invalidations.jl	`0.00% <0.00%> (-86.52%)`	⬇️
src/deep_demos.jl	`0.00% <0.00%> (-57.90%)`	⬇️
src/SnoopCompile.jl	`66.66% <0.00%> (-33.34%)`	⬇️
src/write.jl	`89.47% <0.00%> (-1.84%)`	⬇️
src/visualizations.jl	`0.00% <0.00%> (ø)`
src/parcel_snoopi.jl	`94.11% <0.00%> (+0.28%)`	⬆️
src/parcel_snoopc.jl	`78.72% <0.00%> (+0.34%)`	⬆️

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 · 2021-07-25T18:57:40Z

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)

timholy

LGTM, just one small comment.

A test would be nice, of course!

timholy · 2021-07-25T19:45:32Z

SnoopCompileCore/src/SnoopCompileCore.jl

@@ -23,4 +23,8 @@ if VERSION >= v"1.6.0-DEV.1192"  # https://github.com/JuliaLang/julia/pull/37136
    include("snoopl.jl")
 end

+if VERSION >= v"1.6.0-DEV.1192"  # https://github.com/JuliaLang/julia/pull/37136
+    include("snoop_all.jl")


Can't we just include this macro into SnoopCompileCore/src/snoopl.jl? It's the same condition for including it that we use for@snoopl.

Yeah, good idea. I've just moved it into the same if-block. I guess it doesn't necessarily belong in snoopl.jl because it will snoop all three types of compilation, which is why I put it in its own file.

Can you have another look and let me know if this looks okay? :)

```julia julia> f(x) = 2^x+100 f (generic function with 1 method) julia> tinf, snoopl_csv, snoopl_yaml, snoopc_csv = SnoopCompileCore.@snoop_all "snoop_all-f" f(2) (InferenceTimingNode: 0.003273/0.003523 on Core.Compiler.Timings.ROOT() with 1 direct children, "snoop_all-f.snoopl.csv", "snoop_all-f.snoopl.yaml", "snoop_all-f.snoopc.csv") ```

NHDaly · 2021-07-26T16:13:24Z

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 · 2021-07-26T16:13:42Z

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

NHDaly · 2021-07-26T16:14:00Z

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

bkamins · 2021-07-26T16:40:37Z

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

running this macro swamps my terminal with the output; is this intended?
the two files you give .csv extension are not comma separated (it seems they are tab separated)
could you please guide me what do the numbers in *snoopc.csv file mean (these in the beginning of each line)

Thank you!

bkamins · 2021-07-26T16:46:37Z

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 · 2021-07-26T23:26:54Z

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 · 2021-07-27T06:11:34Z

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.

timholy

Looks quite good. For consistency, let's re-export @snoop_all from SnoopCompile.

timholy · 2021-07-27T12:15:00Z

SnoopCompileCore/src/snoopc.jl

-            while !eof(stdin)
-                Core.eval(Main, deserialize(stdin))
-            end
+            Core.eval(Main, deserialize(IOBuffer(read(stdin))))


Any particular reason for this change? What if stdin is huge? Likewise elsewhere among the changes.

Is it possible that this is the change that causes the closed stream issue? It seems possible that Julia 1.0 is not being as careful as this code.

Alas, this is not responsible for the error -- this was my attempt to fix it, but it didn't help. I can happily revert these changes, yeah.

timholy · 2021-07-27T12:27:02Z

test/snoop_all.jl

@@ -0,0 +1,34 @@
+using Test
+
+using SnoopCompile


Can't be run as a standalone unless either you say using SnoopCompileCore also, or re-export @snoop_all and then drop the module qualifier below.

kimikage · 2024-04-25T01:46:46Z

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.

NHDaly added 2 commits July 25, 2021 13:29

Add pspawn=true to @snoopl to allow staying in the same process!

74371aa

Add @snoop_all to run snoopi, snoopc, snoopl

25c2d6e

NHDaly force-pushed the nhd-snoop-all branch from cabbf64 to 25c2d6e Compare July 25, 2021 17:29

NHDaly mentioned this pull request Jul 25, 2021

inference/LLVM codegen/native codegen time split #251

Open

timholy reviewed Jul 25, 2021

View reviewed changes

NHDaly added 5 commits July 26, 2021 11:15

Add pspawn= option to snoopc; add snoopc to snoop_all

ecdb272

Add basic test for snoop_all.

50c030a

Add docstring to snoop_all

24c8c47

Simplify process spawning code

995ffd2

Add documentation links & example to docstring

c06d84e

NHDaly marked this pull request as ready for review July 26, 2021 23:27

timholy reviewed Jul 27, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

NHDaly commented Jul 25, 2021 •

edited

Loading

codecov bot commented Jul 25, 2021 •

edited

Loading

bkamins commented Jul 25, 2021

timholy left a comment

timholy Jul 25, 2021

NHDaly Jul 26, 2021

NHDaly commented Jul 26, 2021

NHDaly commented Jul 26, 2021

NHDaly commented Jul 26, 2021

bkamins commented Jul 26, 2021

bkamins commented Jul 26, 2021

NHDaly commented Jul 26, 2021

bkamins commented Jul 27, 2021 •

edited

Loading

timholy left a comment

timholy Jul 27, 2021

NHDaly Jul 27, 2021

timholy Jul 27, 2021

kimikage commented Apr 25, 2024

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

Are you sure you want to change the base?

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

Conversation

NHDaly commented Jul 25, 2021 • edited Loading

codecov bot commented Jul 25, 2021 • edited Loading

Codecov Report

bkamins commented Jul 25, 2021

timholy left a comment

Choose a reason for hiding this comment

timholy Jul 25, 2021

Choose a reason for hiding this comment

NHDaly Jul 26, 2021

Choose a reason for hiding this comment

NHDaly commented Jul 26, 2021

NHDaly commented Jul 26, 2021

NHDaly commented Jul 26, 2021

bkamins commented Jul 26, 2021

bkamins commented Jul 26, 2021

NHDaly commented Jul 26, 2021

bkamins commented Jul 27, 2021 • edited Loading

timholy left a comment

Choose a reason for hiding this comment

timholy Jul 27, 2021

Choose a reason for hiding this comment

NHDaly Jul 27, 2021

Choose a reason for hiding this comment

timholy Jul 27, 2021

Choose a reason for hiding this comment

kimikage commented Apr 25, 2024

NHDaly commented Jul 25, 2021 •

edited

Loading

codecov bot commented Jul 25, 2021 •

edited

Loading

bkamins commented Jul 27, 2021 •

edited

Loading