Open Bug 1491946 Opened 11 months ago Updated 8 months ago

Firefox should save heap snapshots with names ending in .fxsnapshot.gz

Categories

(DevTools :: Memory, enhancement, P3)

enhancement

Tracking

(firefox64 fixed)

REOPENED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: jimb, Assigned: jimb)

References

Details

Attachments

(1 file, 2 obsolete files)

Firefox heap snapshots are serialized according to the format described in the Google Protocol Buffer spec devtools/shared/heapsnapshot/CoreDump.proto, compressed with gzip. It would be a bit easier to handle these files (directly, not via the devtools UI) if their file extension reflected their format - gunzip won't nicely decompress files unless their names end in .gz.
Comment on attachment 9009724 [details] [diff] [review]
Save devtools heap snapshots with the extension '.fxsnapshot.gz'.

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

Is it ready for a formal review, Jim ?

::: devtools/client/memory/utils.js
@@ +42,5 @@
>    }
>  
>    if (snapshot.imported) {
> +    // Strip out the extension if it's the expected ".fxsnapshot.gz"
> +    return OS.Path.basename(snapshot.path.replace(/\.fxsnapshot.gz$/, ""));

wondering if we should replace the old extension too. Then the regexp could be:

  /\.fxsnapshot(?:\.gz)?$/

(also note the second dot should be escaped too)
Priority: -- → P3
(In reply to Julien Wajsberg [:julienw] from comment #1)
> wondering if we should replace the old extension too. Then the regexp could
> be:
> 
>   /\.fxsnapshot(?:\.gz)?$/
> 
> (also note the second dot should be escaped too)

Ah, good point. Fixed.
Assignee: nobody → jimb
Attachment #9009724 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #9010309 - Flags: review?(nfitzgerald)
Comment on attachment 9010309 [details] [diff] [review]
Save devtools heap snapshots with the extension '.fxsnapshot.gz'.

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

Thanks!
Attachment #9010309 - Flags: review?(nfitzgerald) → review+
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2defd19a1467
Save devtools heap snapshots with the extension '.fxsnapshot.gz'. r=fitzgen
Keywords: checkin-needed
Backed out changeset 2defd19a1467 (Bug 1491946) for ES lint failure ^^
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56c912e68c10
Backed out changeset 2defd19a1467 for ES lint failure
Hey Jim, do you think you'll be able to update the patch with a fix to the eslint failure?
Thanks !
Flags: needinfo?(felash) → needinfo?(jimb)
Yes, fixing up, and doing a new try push myself...
Flags: needinfo?(jimb)
Fix ESLint failures. Carrying over r=fitzgen.
Attachment #9010309 - Attachment is obsolete: true
Attachment #9011959 - Flags: review+
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9bdc101c70b
Save devtools heap snapshots with the extension '.fxsnapshot.gz'. r=fitzgen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c9bdc101c70b
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Depends on: 1494195
In bug 1494195, it's observed that this patch interacts badly with `nsLocalFileCommon::CreateUnique`, which begins generating filenames like "XXX.fxsnapshot-1.gz", which no longer match the "*.fxsnapshot.gz" pattern.
I like the suggestion from Nicholas, that we only need to expose this filename at download time to the user.
reopening because it's been backed out in bug 1494195.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Julien, would you find the change in this bug useful? I didn't pursue it after the backout because it seemed like something that so few people would be affected by it wasn't worth it. It just happened to be helpful to me in my experiments analyzing the heap snapshots directly.

If there were a single other person who wanted this change, I could definitely fix the problem from bug 1494195.
:)
Flags: needinfo?(felash)
I still find this valuable, although not essential :) I find it useful because the `.gz` suffix works automatically fine with a lot of tools (like good old gunzip that doesn't uncompress files witht .gz by default).

I think we don't need to change the file generation logic at all, but merely change the proposed filename when proposing the download to the user (I suppose this is possible?). This should avoid the problem from bug 1494195.
Flags: needinfo?(felash)
Okay, thanks for the background. The reason I changed the file generation logic was that I was actually calling ChromeUtils.saveHeapSnapshot directly, not going through the devtools panel. I want to call this from within mochitests, etc.
You need to log in before you can comment on or make changes to this bug.