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

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Shu-yu Guo [:shu]]

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.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Handle huge strings in the profile JSON writer.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Remove dead code in the ProfileBuffer and ThreadProfile.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Return UniquePtr<char[]> from profiler_get_profile to avoid double copying.

Comment 4

[Markus Stange [:mstange]]

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!

Comment 5

[Markus Stange [:mstange]]

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)

Comment 6

[Markus Stange [:mstange]]

also, mChunkStart == mChunkList.back() + strlen(mChunkList.back())

Comment 7

[Shu-yu Guo [:shu]]

(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.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da5bcffc2502
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eb1c387ca5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/588c920f963c

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/da5bcffc2502
https://hg.mozilla.org/mozilla-central/rev/7eb1c387ca5f
https://hg.mozilla.org/mozilla-central/rev/588c920f963c