JuliaPerf / PProf.jl

Export Julia profiles to the pprof format
MIT License
155 stars 18 forks source link

Compress on the fly #82

Closed vchuravy closed 12 months ago

vchuravy commented 1 year ago

Fixes #81

NHDaly commented 1 year ago

🤔 hrm tests are failing now, even though we are decoding. i'm not sure why. Any idea?

NHDaly commented 1 year ago

@adnan-alhomssi: This seems like a good idea, and is related to your suggestion that we should be compressing the profiles and heapsnapshots before sending them back from ProfileEndpoints.jl.

Can you have a look and see if you can help us figure out why the tests aren't passing?

NHDaly commented 1 year ago

Request for review: @Drvi

codecov-commenter commented 12 months ago

Codecov Report

Merging #82 (a59d315) into main (45399a8) will increase coverage by 0.06%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
+ Coverage   96.59%   96.66%   +0.06%     
==========================================
  Files           3        3              
  Lines         294      300       +6     
==========================================
+ Hits          284      290       +6     
  Misses         10       10              
Files Coverage Δ
src/Allocs.jl 92.40% <100.00%> (+0.19%) :arrow_up:
src/PProf.jl 99.19% <100.00%> (+0.01%) :arrow_up:
src/flamegraphs.jl 96.90% <100.00%> (+0.06%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Drvi commented 11 months ago

Will this break reading profiles from older PProf.jl versions? Typically with text files, one can sniff the gzip "magic number", but with ProtoBuf I think we need to check first if there are possible collisions with the encoded message.

NHDaly commented 11 months ago

Thankfully, i don't think PProf.jl ever reads profiles, we just hand them over to pprof, which seems to be able to read them fine, in either version.

So i think it's okay? What concern did you have in mind?

Drvi commented 11 months ago

Ah if we don't need to read it on Julia side, then it's fine:+1: sorry for the noise