Clean up the profiler JSON writer to handle huge strings and avoid extra copying

RESOLVED FIXED in Firefox 41

Status

()

Core
Gecko Profiler
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: shu, Unassigned)

Tracking

unspecified
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
Currently when profiling e10s we sometimes crash when streaming the JSON because the content process profile is written as a single string, which may overflow a single chunk of the ChunkedJSONWriteFunc.

Also take the opportunity to have getProfile use the chunked writer instead of a stringstream.
(Reporter)

Comment 1

3 years ago
Created attachment 8607839 [details] [diff] [review]
Handle huge strings in the profile JSON writer.
Attachment #8607839 - Flags: review?(mstange)
(Reporter)

Comment 2

3 years ago
Created attachment 8607840 [details] [diff] [review]
Remove dead code in the ProfileBuffer and ThreadProfile.
Attachment #8607840 - Flags: review?(mstange)
(Reporter)

Comment 3

3 years ago
Created attachment 8607842 [details] [diff] [review]
Return UniquePtr<char[]> from profiler_get_profile to avoid double copying.
Attachment #8607842 - Flags: review?(mstange)
Attachment #8607840 - Flags: review?(mstange) → review+
Comment on attachment 8607842 [details] [diff] [review]
Return UniquePtr<char[]> from profiler_get_profile to avoid double copying.

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

Good!
Attachment #8607842 - Flags: review?(mstange) → review+
Comment on attachment 8607839 [details] [diff] [review]
Handle huge strings in the profile JSON writer.

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

I have no recollection of reviewing this class before, but apparently I r+ed it in bug 1154115...

I have a few requests that are unrelated to this bug, so it'd be cool if you could fix them in a new bug:
 - methods should go before member variables
 - the implementations of the methods are long enough to be moved into the .cpp file
 - a comment above the class for why we want to store this in chunks in the first place would be nice
 - each of the member variables could need a comment
 - a list of invariants (in a comment) might be useful, e.g.
    - mChunkEnd == mChunkList.back() + mChunkLengths.back()
    - mChunkStart >= mChunkList.back() && mChunkStart < mChunkEnd
    - *mChunkStart == '\0'
   (please double-check if these are true)
Attachment #8607839 - Flags: review?(mstange) → review+
also, mChunkStart == mChunkList.back() + strlen(mChunkList.back())
(Reporter)

Comment 7

3 years ago
(In reply to Markus Stange [:mstange] from comment #5)
> Comment on attachment 8607839 [details] [diff] [review]
> Handle huge strings in the profile JSON writer.
> 
> Review of attachment 8607839 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have no recollection of reviewing this class before, but apparently I r+ed
> it in bug 1154115...
> 
> I have a few requests that are unrelated to this bug, so it'd be cool if you
> could fix them in a new bug:
>  - methods should go before member variables
>  - the implementations of the methods are long enough to be moved into the
> .cpp file
>  - a comment above the class for why we want to store this in chunks in the
> first place would be nice
>  - each of the member variables could need a comment
>  - a list of invariants (in a comment) might be useful, e.g.
>     - mChunkEnd == mChunkList.back() + mChunkLengths.back()
>     - mChunkStart >= mChunkList.back() && mChunkStart < mChunkEnd
>     - *mChunkStart == '\0'
>    (please double-check if these are true)

Sure thing, filed bug 1168265.
https://hg.mozilla.org/mozilla-central/rev/da5bcffc2502
https://hg.mozilla.org/mozilla-central/rev/7eb1c387ca5f
https://hg.mozilla.org/mozilla-central/rev/588c920f963c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.