Import dhat profiles #3128

gregtatum · 2021-01-12T18:08:21Z

Edit: This is now ready to review. Please ignore the first commit, as it's from #3222. Also note the code diff size is from snapshot churn. I flagged both of you for review since I don't know your workload, but I only need one person to do the review.

It adds support for importing dhat memory profiles, specifically from dhat-rs. The file format is part of the valgrind tools. We're using this tool as part of our memory benchmarks for ICU4X, and I would like to be able to link to profiles if possible. I couldn't really figure out how to read the built in viewer, which is a bit awkward.

Here is a profile to test with:
dhat-heap.json.zip

Here is a deploy preview

codecov · 2021-03-23T15:58:09Z

Codecov Report

Merging #3128 (12dec49) into main (5707f11) will increase coverage by 0.04%.
The diff coverage is 96.72%.

❗ Current head 12dec49 differs from pull request most recent head 8406440. Consider uploading reports for the commit 8406440 to get more accurate results

@@            Coverage Diff             @@
##             main    #3128      +/-   ##
==========================================
+ Coverage   88.76%   88.81%   +0.04%     
==========================================
  Files         255      256       +1     
  Lines       20808    20930     +122     
  Branches     5331     5346      +15     
==========================================
+ Hits        18470    18588     +118     
- Misses       2162     2166       +4     
  Partials      176      176

Impacted Files	Coverage Δ
src/profile-logic/import/art-trace.js	`91.26% <ø> (ø)`
src/selectors/per-thread/thread.js	`94.65% <60.00%> (-1.38%)`	⬇️
src/profile-logic/import/dhat.js	`98.21% <98.21%> (ø)`
src/actions/profile-view.js	`85.20% <100.00%> (ø)`
src/profile-logic/import/chrome.js	`94.72% <100.00%> (+0.01%)`	⬆️
src/profile-logic/process-profile.js	`91.13% <100.00%> (+0.05%)`	⬆️
src/components/tooltip/CallNode.js	`87.05% <0.00%> (ø)`
src/components/timeline/TrackNetwork.js	`94.79% <0.00%> (ø)`
src/components/tooltip/Marker.js	`97.32% <0.00%> (+0.02%)`	⬆️

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 5707f11...8406440. Read the comment docs.

src/test/unit/profile-conversion.test.js

gregtatum · 2021-03-26T20:25:35Z

I forgot to remove the [wip] tag here, and I rebased over #3222.

.gitattributes

julienw

Thanks for this addition, this looks very useful!

When I try the deploy preview, I get a hang quite easily, by clicking here and there in the tracks, here is a profile I got from my local dev: https://share.firefox.dev/2O2mN1c

Also I don't see anything in the tracks, is that expected?

Otherwise this looks reasonable, I left a few comments with mostly questions and comments. I'm not approving mainly because of the 2 above problems, but the code looks fine otherwise.

Thanks!

.gitattributes

src/types/profile.js

src/profile-logic/import/dhat.js

src/selectors/per-thread/stack-sample.js

julienw

marking as "request changes" just for the sake of having the correct status in the list of PR.

gregtatum · 2021-04-12T19:46:07Z

Thanks. I still need to look at the feedback, but I've been busy with the 0.2 release on ICU4X. I still plan on looking at this when I get a bit of time.

gregtatum · 2021-07-23T14:50:55Z

I accidentally clobbered a Gecko build, so I took the time to finally get back to addressing the review on this 😅

julienw · 2021-07-28T18:05:22Z

@gregtatum have you seen this comment when you handled the review comments?

When I try the deploy preview, I get a hang quite easily, by clicking here and there in the tracks, here is a profile I got from my local dev: https://share.firefox.dev/2O2mN1c

Also I don't see anything in the tracks, is that expected?

This still seems to happen.

Thanks!

gregtatum · 2021-08-03T18:41:16Z

I think I missed your hang comment.

I'm not reproducing, and I'm a bit unclear on the STR. I'm clicking around but things seem fine:

https://deploy-preview-3128--perf-html.netlify.app/public/cag9r7d4qj8chjadcw63ctv73cdq7rhdc0p6v1g/flame-graph/?globalTrackOrder=7w91w506&hiddenGlobalTracks=1w5&hiddenLocalTracksByPid=28213-02w4&localTrackOrderByPid=28213-602w415~28275-0~28424-0~28373-0~28316-0~28982-0~28449-0&range=3461m16148&thread=0&timelineType=cpu-category&v=6

Also I don't see anything in the tracks, is that expected?

This is expected, as there is no timeline data. It's only the summary.

julienw · 2021-08-04T14:17:24Z

Thanks a lot for addressing the issue of reusing the thread object!

Regarding the hang, this is happening when I click around in the timeline, mostly when I click on the tracks to select them, but I got it also when I clicked always in the same track. The console says: Uncaught out of memory.
I made you another profile in development mode so that you have the function names:
https://share.firefox.dev/3yoOQtR
This is on my newer much faster computer, and here the hang stops after a few seconds (still 19 seconds!). I think that in my previous computer this wouldn't stop, probably the OOM wouldn't come so fast.

I'd be happy merging this and you can handle this issue in a separate PR, especially that you'll be one of the only users of the feautre ;-) Up to you.

julienw · 2021-08-05T09:00:21Z

src/actions/profile-view.js

    const newSelectedCallNode =
-      newSelectedStack === null
+      newSelectedStack === null || newSelectedStack === undefined


thanks for the fix !!
curious that it wasn't deterministic and only happened sometimes...

Yeah, I think the real fix would be to check how the sample index is being generated, as it should be a null if there are no samples.

However, that was a lot more work with more call sites, this fix was expedient.

* Support dhat imports (see #3128 for more information) * Remove mentions of the old add-on * A few fixes in the tooltips dimensioning * Prepare support for network requests cancellations

gregtatum marked this pull request as draft January 12, 2021 18:08

This was referenced Mar 15, 2021

Fix the call node max depth calculation #3221

Merged

Make categories optional in imported profiles #3222

Merged

gregtatum force-pushed the dhat branch from ed550bd to 400c626 Compare March 23, 2021 15:54

gregtatum commented Mar 23, 2021

View reviewed changes

src/test/unit/profile-conversion.test.js Show resolved Hide resolved

gregtatum marked this pull request as ready for review March 23, 2021 16:07

gregtatum requested review from julienw and canova March 23, 2021 16:07

gregtatum changed the title ~~[wip] Import dhat profiles~~ Import dhat profiles Mar 26, 2021

gregtatum force-pushed the dhat branch from 400c626 to 9762327 Compare March 26, 2021 20:23

gregtatum commented Mar 26, 2021

View reviewed changes

.gitattributes Show resolved Hide resolved

julienw reviewed Mar 29, 2021

View reviewed changes

julienw requested changes Apr 12, 2021

View reviewed changes

canova removed their request for review May 3, 2021 08:20

gregtatum force-pushed the dhat branch from 9762327 to 0eb7b0d Compare July 23, 2021 14:48

gregtatum requested a review from julienw July 23, 2021 14:50

gregtatum added 8 commits August 3, 2021 15:02

Add a dhat importer

1556cb6

Use native allocations by default if no samples exist

8f5a85a

Add importedFrom field to the profile

700ee6b

Add a dhat test

7dd806b

Mark profile-conversion.test.js.snap as binary

a08b2b5

Re-generate the snapshots for the rebase

000bb7e

Address review comments

1f74c79

Create new copies of the threads in dhat importer

0f8964e

gregtatum force-pushed the dhat branch from ff721aa to 0f8964e Compare August 3, 2021 20:04

Check for undefined selected stack

12dec49

julienw reviewed Aug 5, 2021

View reviewed changes

julienw approved these changes Aug 5, 2021

View reviewed changes

Merge branch 'main' into dhat

8406440

julienw merged commit a3cfe26 into firefox-devtools:main Aug 5, 2021

julienw mentioned this pull request Aug 9, 2021

Deploy August 9, 2021 #3490

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import dhat profiles #3128

Import dhat profiles #3128

gregtatum commented Jan 12, 2021 •

edited

Loading

codecov bot commented Mar 23, 2021 •

edited

Loading

gregtatum commented Mar 26, 2021

julienw left a comment

julienw left a comment

gregtatum commented Apr 12, 2021

gregtatum commented Jul 23, 2021

julienw commented Jul 28, 2021

gregtatum commented Aug 3, 2021

julienw commented Aug 4, 2021

julienw Aug 5, 2021

gregtatum Aug 6, 2021

gregtatum Aug 6, 2021

Import dhat profiles #3128

Import dhat profiles #3128

Conversation

gregtatum commented Jan 12, 2021 • edited Loading

codecov bot commented Mar 23, 2021 • edited Loading

Codecov Report

gregtatum commented Mar 26, 2021

julienw left a comment

Choose a reason for hiding this comment

julienw left a comment

Choose a reason for hiding this comment

gregtatum commented Apr 12, 2021

gregtatum commented Jul 23, 2021

julienw commented Jul 28, 2021

gregtatum commented Aug 3, 2021

julienw commented Aug 4, 2021

julienw Aug 5, 2021

Choose a reason for hiding this comment

gregtatum Aug 6, 2021

Choose a reason for hiding this comment

gregtatum Aug 6, 2021

Choose a reason for hiding this comment

gregtatum commented Jan 12, 2021 •

edited

Loading

codecov bot commented Mar 23, 2021 •

edited

Loading