Improve Spliceable{,Chunked}JSONWriter maintainability
Categories
(Core :: Gecko Profiler, task, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox82 | --- | fixed |
People
(Reporter: mozbugz, Assigned: mozbugz)
References
Details
Attachments
(3 files)
SpliceableJSONWriter and SpliceableChunkedJSONWriter are still playing with pointers, sometimes to copy data and sometimes to move data.
It would be good to improve this to reduce the cognitive maintenance overload, before I make more substantial changes to deal with OOMs...
Assignee | ||
Comment 1•4 years ago
|
||
SpliceableChunkedJSONWriter::ChunkedWriteFunc
returns a ChunkedJSONWriteFunc*
, which is never null and is either used to:
- Copy data.
- Or take ownership of the chunks.
In the first case, ChunkedWriteFunc()
now returns a const ChunkedJSONWriteFunc&
(notice "const &"), so only const members may be used to copy the data.
In the second case, a new function TakeChunkedWriteFunc()
returns ChunkedJSONWriteFunc&&
(notice "&&"), so it's clear that its chunks can be taken away. Some DEBUG
assertions help ensure that it's not used anymore after that.
TakeAndSplice()
now takes a ChunkedJSONWriteFunc&&
.
All callers have been updated to the more appropriate functions.
Assignee | ||
Comment 2•4 years ago
|
||
In most calls to SpliceableJSONWriter::Splice(const char*)
:
- The data comes from a
ChunkedJSONWriteFunc
and is copied to a new buffer, which is then copied again throughWrite()
. Instead we can copy the data directly from theChunkedJSONWriteFunc
; and this is a nice complement toTakeAndSplice()
below. - Or the length is already known, so we can pass it to a new
Splice(const char*, size_t)
, which forwards it toWrite(const char*, size_t)
, saving onestrlen
call.
Depends on D87702
Assignee | ||
Comment 3•4 years ago
|
||
Normally I believe it's good to keep functionally-distinct code in separate functions when appropriate.
However in this case, this ChunkedJSONWriteFunc::GetTotalLength()
function was only used internally, so it's easy to hide it. It is not very big, so it's less important to keep it separate. And its code (going through all chunks to collect sizes) is similar to the code that follows (going through all chunks to collect their content), so it makes some sense to have these similar things in the same place; if one day this chunking change, both loops would probably have to be modified at the same time.
More importantly to justify this change: It was including the final null terminator, which was a bit inconsistent with the usual Length()
functions in string containers.
Now that code is only used where absolutely necessary (before allocating an output buffer), so it rightly accounts for the null terminator that is added at the end of the contents.
And in upcoming bug 1612799, ChunkedJSONWriteFunc
will have a new "retired" state (e.g., after an internal memory allocation failed) in which case a public GetTotalLength()
function could give misleading results -- should it be 0 (nothing to output), 1 (only the null terminator), 5 ("null"), or the length of some error message? So I think it's better not to expose such an ambiguous function.
It could of course be re-introduced if new needs arise in the future.
Depends on D87703
Pushed by gsquelart@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/206619e0dcd3 Clarify accesses to SpliceableChunkedJSONWriter's WriteFunc - r=canaltinova https://hg.mozilla.org/integration/autoland/rev/7d47f805799f Replace SpliceableJSONWriter::Splice(const char*) with better calls where possible - r=canaltinova https://hg.mozilla.org/integration/autoland/rev/e9a109b817cc Fold GetTotalLength into its only caller CopyDataIntoLazilyAllocatedBuffer - r=canaltinova
Comment 5•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/206619e0dcd3
https://hg.mozilla.org/mozilla-central/rev/7d47f805799f
https://hg.mozilla.org/mozilla-central/rev/e9a109b817cc
Description
•