Closed
Bug 1166492
Opened 9 years ago
Closed 9 years ago
Clean up the profiler JSON writer to handle huge strings and avoid extra copying
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: shu, Unassigned)
Details
Attachments
(3 files)
2.09 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
5.39 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
9.99 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Attachment #8607839 -
Flags: review?(mstange)
Reporter | ||
Comment 2•9 years ago
|
||
Attachment #8607840 -
Flags: review?(mstange)
Reporter | ||
Comment 3•9 years ago
|
||
Attachment #8607842 -
Flags: review?(mstange)
Updated•9 years ago
|
Attachment #8607840 -
Flags: review?(mstange) → review+
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Comment 6•9 years ago
|
||
also, mChunkStart == mChunkList.back() + strlen(mChunkList.back())
Reporter | ||
Comment 7•9 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/integration/mozilla-inbound/rev/da5bcffc2502 https://hg.mozilla.org/integration/mozilla-inbound/rev/7eb1c387ca5f https://hg.mozilla.org/integration/mozilla-inbound/rev/588c920f963c
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
Closed: 9 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.
Description
•