1214070 - HeapAnalysesWorker should support diffing census reports

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Diagram of what is needed here, chronological order starting at top and ending at the bottom:

-----------8<------------8<-------------8<----------------


HeapAnalysesClient                           HeapAnalysesWorker

(snapshot1, snapshot2, breakdown) --------------> 

                                             takes a census of snapshot1

                                             takes a census of snapshot2

                                             simultaneously walk census
                                             reports, record difference
                                             between the two in delta-report

         <---------------------------------- delta-report


-----------8<------------8<-------------8<----------------

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Should this be done in C++ in Debugger.Memory APIs, or can this be handled in JS in the worker itself? We can probably implement the same `asTreeNode` option as bug 1214231, if we generate a diff report, and run that through CensusTreeNode.

If this is ok to be done in JS, as it's O(n) and depending on the breakdown options, (could be really small, but maybe something like allocation sites would be too much), I can take care of it.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I don't think this needs to be pushed down into the platform. I am planning on making this a different worker request/response, rather than an option on the existing request/response because I think it is different enough to warrant that. I have a WIP already, will get a first draft up soon.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: WIP: Add support for diffing census reports to HeapAnalysesWorker

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Any thoughts on the UI/UX for this?

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Oops, meant to type more. We'd need a way to select 2 snapshots and diff them, and then what is the outcome of that -- possibly just another snapshot (that could have negative values), with numbers being + or - (color coded?)?

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Yes, it will generate another report which contains the delta number/size of things and has the same structure (as specifified by the breakdown).

Not sure what the UI for selecting two snapshots should look like, but the view of the diff should be pretty much the same as viewing a single census report. Yes we should add +/- signs, I like the idea of coloring too.

We should probably sort by abs(delta) in this case, at least at first. And eventually be able to choose sorting by biggest (most new things in census2 that didn't exist in census1) and smallest (things that existed in census1 but no longer exist in census2) as well.

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

For a similar reference, IE11's tools[0], each snapshot contains an overall diff from the first snapshot, although this is aggregate data.

[0] https://i-msdn.sec.s-msft.com/dynimg/IC769141.png

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add support for diffing census reports to HeapAnalysesWorker

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=798b3d27f3ee

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8674281 [details] [diff] [review]
Add support for diffing census reports to HeapAnalysesWorker

Review of attachment 8674281 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/33359eec0505

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/33359eec0505