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

Move `flatten()` to SnoopCompileCore. #310

Closed NHDaly closed 1 year ago

NHDaly commented 1 year ago

At RelationalAI, we are turning on always-on @snoopi_deep profiling in our production servers. For this, we want to add a dependency on SnoopCompileCore, but not on SnoopCompile, since SnoopCompile adds many dependencies for visualization utilities, such as FlameGraphs.jl.

We think that flatten() is a small enough and simple enough utility function that it could easily live in SnoopCompileCore. Does that sound reasonable to you as well?

Thanks! :)

NHDaly commented 1 year ago

CC: @sacha0 and @vilterp for review.

codecov[bot] commented 1 year ago

Codecov Report

Base: 85.47% // Head: 83.99% // Decreases project coverage by -1.48% :warning:

Coverage data is based on head (6b8b8c8) compared to base (ad792af). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #310 +/- ## ========================================== - Coverage 85.47% 83.99% -1.49% ========================================== Files 17 17 Lines 2162 2162 ========================================== - Hits 1848 1816 -32 - Misses 314 346 +32 ``` | [Impacted Files](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/310?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy) | Coverage Δ | | |---|---|---| | [src/parcel\_snoopi\_deep.jl](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/310/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy#diff-c3JjL3BhcmNlbF9zbm9vcGlfZGVlcC5qbA==) | `89.77% <ø> (-0.14%)` | :arrow_down: | | [SnoopCompileCore/src/snoopi\_deep.jl](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/310/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy#diff-U25vb3BDb21waWxlQ29yZS9zcmMvc25vb3BpX2RlZXAuamw=) | `93.54% <100.00%> (+1.54%)` | :arrow_up: | | [SnoopPrecompile/src/SnoopPrecompile.jl](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/310/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy#diff-U25vb3BQcmVjb21waWxlL3NyYy9Tbm9vcFByZWNvbXBpbGUuamw=) | `81.25% <0.00%> (-6.25%)` | :arrow_down: | | [src/invalidations.jl](https://codecov.io/gh/timholy/SnoopCompile.jl/pull/310/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy#diff-c3JjL2ludmFsaWRhdGlvbnMuamw=) | `77.80% <0.00%> (-6.11%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tim+Holy)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

NHDaly commented 1 year ago

🤔 the test failures look unrelated, from what i can see:

snoopi: Test Failed at /Users/runner/work/SnoopCompile.jl/SnoopCompile.jl/test/snoopi.jl:132
  Expression: any((str->begin
            occursin("kwftype", str)
        end), FK)
Stacktrace:
 [1] macro expansion
   @ ~/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:477 [inlined]
 [2] macro expansion
   @ ~/work/SnoopCompile.jl/SnoopCompile.jl/test/snoopi.jl:132 [inlined]
 [3] macro expansion
   @ ~/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:1496 [inlined]
 [4] top-level scope
   @ ~/work/SnoopCompile.jl/SnoopCompile.jl/test/snoopi.jl:97
┌ Warning: Body method was not toplevel-inferred, test skipped
└ @ Main ~/work/SnoopCompile.jl/SnoopCompile.jl/test/snoopi.jl:143
snoopi: Test Failed at /Users/runner/work/SnoopCompile.jl/SnoopCompile.jl/test/snoopi.jl:153
  Expression: any((str->begin
            occursin("kwftype", str)
        end), FK)
Stacktrace:
 [1] macro expansion
   @ ~/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:477 [inlined]
 [2] macro expansion
   @ ~/work/SnoopCompile.jl/SnoopCompile.jl/test/snoopi.jl:153 [inlined]
 [3] macro expansion
   @ ~/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:1496 [inlined]
 [4] top-level scope
   @ ~/work/SnoopCompile.jl/SnoopCompile.jl/test/snoopi.jl:97
snoopi: Test Failed at /Users/runner/work/SnoopCompile.jl/SnoopCompile.jl/test/snoopi.jl:[182](https://github.com/timholy/SnoopCompile.jl/actions/runs/3347452311/jobs/5545449984#step:8:185)
  Expression: any((str->begin
            occursin("Tuple{Core.kwftype(typeof(genkw2)),NamedTuple{(:b,),$(SP)Tuple{String}},typeof(genkw2)}", str)
        end), FK)
Stacktrace:
 [1] macro expansion
   @ ~/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:477 [inlined]
 [2] macro expansion
   @ ~/work/SnoopCompile.jl/SnoopCompile.jl/test/snoopi.jl:182 [inlined]
 [3] macro expansion
   @ ~/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:1496 [inlined]
 [4] top-level scope
   @ ~/work/SnoopCompile.jl/SnoopCompile.jl/test/snoopi.jl:97
timholy commented 1 year ago

Not quite sure what to do here. I was trying to keep SnoopCompileCore to be about data-collection only.

But I get why you'd want something slimmer. The key principle is that SnoopCompileCore can't extend any Base methods (it cannot itself be a source of any invalidations), and obviously this doesn't violate that. My only concern is that this opens the door to moving other things, too, and the end result could be confusing to users. For example, the printing from flatten is probably pretty awful without the show methods, right? But the show methods can't move to SnoopCompileCore.

I'll think about this for another day or two and then decide about merging.

NHDaly commented 1 year ago

Yeah, Tim, I had similar concerns.. We ended up just extracting this method out into our own codebase, which seems fine for now. I think it's up to you about how to proceed. 🤷

timholy commented 1 year ago

I think in the absence of strong pressure to move it, I'd rather keep the clean "collection vs analysis" separation. But I'm happy to continue to brainstorm about this as we move forward. I totally get where you're coming from on this.

NHDaly commented 1 year ago

sounds right, thanks! :)