Closed Bug 1660177 Opened 4 years ago Closed 4 years ago

Improve Spliceable{,Chunked}JSONWriter maintainability

Categories

(Core :: Gecko Profiler, task, P2)

task

Tracking

()

RESOLVED FIXED
82 Branch
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...

SpliceableChunkedJSONWriter::ChunkedWriteFunc returns a ChunkedJSONWriteFunc*, which is never null and is either used to:

  1. Copy data.
  2. 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.

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 through Write(). Instead we can copy the data directly from the ChunkedJSONWriteFunc; and this is a nice complement to TakeAndSplice() below.
  • Or the length is already known, so we can pass it to a new Splice(const char*, size_t), which forwards it to Write(const char*, size_t), saving one strlen call.

Depends on D87702

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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: