Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add V8 .heapprofile importer #4467

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

krsh732
Copy link
Contributor

@krsh732 krsh732 commented Feb 9, 2023

Based on #4463

The first commit should display the same data as Chrome/Edge DevTools and VS Code do for .heapprofiles.
V8 Heap profiles have a samples field containing allocation samples. However, as far as I am aware, none of the existing tools actually use that field (properly). Chrome/Edge have an experimental feature to use samples. While this works when recording the heapprofile, the experimental feature breaks importing profiles and end up displaying nothing at all (even the call tree stops working).

In the 2nd commit I tried to use samples, but I'd like reviewers opinion on it and how to proceed:

  • Would it be too misleading to use this field? While it allows you to filter allocations in the timeline, it technically isn't time at all... The heapprofiles only save the ordinal, not the timestamps used in Chrome's TimelineOverview when recording.
  • There are differences between the sizes reported in the profile tree vs samples (see root total on deploy preview links for example).
  • To combat the differences, should we have an extra track, or does that make things far too confusing? One which uses only the self sizes from the tree, and the other from the sample?

Additional questions for reviewers:

  1. Are the categories OK?
  2. Should I be using the resourceTable similar to the chrome importer instead of pushing -1 like dhat?
  3. Is there any other information that I could/should be populating in the profile for the importer?

Things left to do:

  • Add tests
  • Touch Home--load-files-from-other-tools in app.ftl?

Fixes #4461

Deploy previews with samples: Chrome profile | Node profile
Deploy previews without samples: Chrome profile | Node profile
Original .heapprofiles used for previews: heapprofiles.zip

┆Issue is synchronized with this Jira Task

@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Patch coverage: 27.85% and project coverage change: -0.34 ⚠️

Comparison is base (16e5c34) 88.63% compared to head (a85e9c0) 88.29%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4467      +/-   ##
==========================================
- Coverage   88.63%   88.29%   -0.34%     
==========================================
  Files         293      294       +1     
  Lines       25981    26101     +120     
  Branches     6996     7025      +29     
==========================================
+ Hits        23027    23047      +20     
- Misses       2748     2836      +88     
- Partials      206      218      +12     
Impacted Files Coverage Δ
src/profile-logic/import/v8-heap-profile.js 10.09% <10.09%> (ø)
src/components/shared/StackSettings.js 92.50% <66.66%> (-2.24%) ⬇️
src/profile-logic/process-profile.js 90.96% <66.66%> (-0.12%) ⬇️
src/profile-logic/profile-data.js 93.67% <93.75%> (+<0.01%) ⬆️
src/selectors/per-thread/composed.js 100.00% <100.00%> (+3.12%) ⬆️
src/selectors/per-thread/thread.js 94.73% <100.00%> (+0.03%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@julienw
Copy link
Contributor

julienw commented Feb 17, 2023

Hey! Just a quick note that we've seen your PR but just didn't have the chance to look at it during this week. It's still on our radar, thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support node's heap profiling
2 participants