Change the order of merging on about:profiler.

Change the order of merging on about:profiler.

When diff-ing two snapshots, do the merge *before* doing the diff. In practice this doesn't usually yield different results, but it is possible. Merging before diffing makes more sense though, and avoids some duplicated work.

BUG=105301
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113454

[eroman, 2011-11-23 22:33:28]

http://codereview.chromium.org/8676023/diff/1/chrome/browser/resources/profil...
File chrome/browser/resources/profiler.js (right):

http://codereview.chromium.org/8676023/diff/1/chrome/browser/resources/profil...
chrome/browser/resources/profiler.js:1251: updateMergedDataSoon_: function() {
this is just a rename "flatData" -> "mergedData"

[jar (doing other things), 2011-12-06 01:54:36]

LGTM

Nits below to consider... but you can land with or without the changes.

http://codereview.chromium.org/8676023/diff/1/chrome/browser/resources/profil...
File chrome/browser/resources/profiler.js (left):

http://codereview.chromium.org/8676023/diff/1/chrome/browser/resources/profil...
chrome/browser/resources/profiler.js:1302: * difference between the two
snapshots and update flatData_.
nit: It would be good to update the comment, rather than discarding it.

http://codereview.chromium.org/8676023/diff/1/chrome/browser/resources/profil...
File chrome/browser/resources/profiler.js (right):

http://codereview.chromium.org/8676023/diff/1/chrome/browser/resources/profil...
chrome/browser/resources/profiler.js:955: function subtractSnapshots(data1,
data2, columnsToExclude) {
nit: rather than data1, and data2, perhaps data_new and data_old would make it
clearer which was being subtracted from the which.  For instance, we are
guaranteed than all keys found in data_old also exist in data_new (even if they
have value zero in many fields).

http://codereview.chromium.org/8676023/diff/1/chrome/browser/resources/profil...
chrome/browser/resources/profiler.js:1242: return;
This may be a dangerous optimization. I think it is ok... but I doubt it is
worth it.  I'm not clear if it is correct for example if a belated snapshot
arrives, and is an intermediare of the selected pair of snapshots.

I was especially worried given the outstanding bug on the Mac (why I'm
commenting).

[eroman, 2011-12-06 19:21:25]

thanks for the review

http://codereview.chromium.org/8676023/diff/1/chrome/browser/resources/profil...
File chrome/browser/resources/profiler.js (left):

http://codereview.chromium.org/8676023/diff/1/chrome/browser/resources/profil...
chrome/browser/resources/profiler.js:1302: * difference between the two
snapshots and update flatData_.
On 2011/12/06 01:54:36, jar wrote:
> nit: It would be good to update the comment, rather than discarding it.

Done.

(I initially omitted the comment since I felt the new function name adequately
explained things.)

http://codereview.chromium.org/8676023/diff/1/chrome/browser/resources/profil...
File chrome/browser/resources/profiler.js (right):

http://codereview.chromium.org/8676023/diff/1/chrome/browser/resources/profil...
chrome/browser/resources/profiler.js:955: function subtractSnapshots(data1,
data2, columnsToExclude) {
On 2011/12/06 01:54:36, jar wrote:
> nit: rather than data1, and data2, perhaps data_new and data_old would make it
> clearer which was being subtracted from the which.  For instance, we are
> guaranteed than all keys found in data_old also exist in data_new (even if
they
> have value zero in many fields).  

I'll leave this to a follow-up change (since it touches quite a few lines).

http://codereview.chromium.org/8676023/diff/1/chrome/browser/resources/profil...
chrome/browser/resources/profiler.js:1242: return;
On 2011/12/06 01:54:36, jar wrote:
> This may be a dangerous optimization. I think it is ok... but I doubt it is
> worth it.  I'm not clear if it is correct for example if a belated snapshot
> arrives, and is an intermediare of the selected pair of snapshots.
> 
> I was especially worried given the outstanding bug on the Mac (why I'm
> commenting).

I'll leave this to a follow-up change (since I want to keep this change focused
on the merge refactoring for simplicity).

It is safe with the current code, but I agree once we are doing min/max for
snapshots we will need to re-consider every snapshot between a range.

[eroman, 2011-12-06 22:11:32]

@estade: Can I get an LGTM for OWNERS approval?

[eroman, 2011-12-07 19:16:22]

Removed estade as reviewer, since I have now added myself as an OWNER for
profiler.

(Discussed with jhawkings, he will be doing some reviews to make sure I am
adhering to style).