1784812 - JSONWriter refactoring and de-duplication

Status: RESOLVED FIXED

(Core :: MFBT, task, P3)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Some ideas for making JSONWriter better:

• WriteFunc() could return a reference instead of a pointer (so that there's no doubt that it's always there.)
• JSONWriter could just reference a JSONWriteFunc, instead of always owning it, this could remove some allocations, and the user could choose how to manage their JSONWriteFunc.
• Most JSONWriteFunc's append characters to a string (owned or referenced), all these could be replaced with a couple of shared implementations, making it easier to modify all the JSONWriterFuncs later if needed.
• Some more finals could be sprinkled, to hopefully help the compiler de-virtualize some calls.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1784812 - JSONWriter::WriteFunc() returns a reference - r?canaltinova

mWriter is never null (and lots of calls just dereference it without checking),
so we may as well enforce it:

• The constructor MOZ_RELEASE_ASSERTs that it's not null.
• The accessor WriteFunc() returns a reference instead of a scary raw pointer.

(Note that we can't make mWriter a NotNull<...>, because the next patch will
give the option to keep that owning pointer null.)

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1784812 - JSONWriter may optionally not own its writer - r?canaltinova

mWriter is now a reference, and the ownership is optional through a separate
member variable that could stay null.
User can now choose to keep the JSONWriteFunc on their stack, which saves a
heap allocation, and makes it easier to access the concrete JSONWriteFunc
implementation directly (instead of through WriteFunc()).

Depends on D154616

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1784812 - Use common JSONWriteFuncs when writing to a string - r?canaltinova

Most users of JSONWriter want to fill a string, so instead of having all these
similar implementations, we now have central reusable implementations:

• JSONStringWriteFunc contains a string and writes to it.
• JSONStringRefWriteFunc references a string and writes to it. This is most
  useful when the string already exists somewhere, or needs to be returned from
  a function (so we avoid another conversion when returning).

Depends on D154617

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1784812 - Make all JSONWriteFunc-derived classes and their overriden methods final - r?canaltinova

All JSONWriteFuncs are effectively final, this patch enforces that, hopefully
helping the compiler to de-virtualize some calls.

Depends on D154618

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8506496d8f2
JSONWriter::WriteFunc() returns a reference - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/9f01bf89c583
JSONWriter may optionally not own its writer - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/d2568bc2f8a6
Use common JSONWriteFuncs when writing to a string - r=canaltinova,media-playback-reviewers,alwu
https://hg.mozilla.org/integration/autoland/rev/c9998c927079
Make all JSONWriteFunc-derived classes and their overriden methods final - r=canaltinova

Comment 6

[Iulian Moraru]

Backed out for causing build bustages on DDMediaLogs.

Push with failures

Failure log

Backout link

[task 2022-08-17T02:43:03.872Z] 02:43:03     INFO -  gmake[4]: Entering directory '/builds/worker/workspace/obj-build/dom/media/doctor'
[task 2022-08-17T02:43:03.875Z] 02:43:03     INFO -  /builds/worker/fetches/clang/bin/clang++ --sysroot /builds/worker/fetches/sysroot-x86_64-linux-gnu -std=gnu++17 -o Unified_cpp_dom_media_doctor0.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/dom/media/doctor -I/builds/worker/workspace/obj-build/dom/media/doctor -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wno-error=deprecated -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-missing-braces -Wno-unknown-warning-option -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fno-exceptions -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -gdwarf-4 -Os -fno-omit-frame-pointer -funwind-tables -Werror -fno-strict-aliasing -ffp-contract=off  -MD -MP -MF .deps/Unified_cpp_dom_media_doctor0.o.pp   Unified_cpp_dom_media_doctor0.cpp
[task 2022-08-17T02:43:03.875Z] 02:43:03     INFO -  In file included from Unified_cpp_dom_media_doctor0.cpp:74:
[task 2022-08-17T02:43:03.875Z] 02:43:03    ERROR -  /builds/worker/checkouts/gecko/dom/media/doctor/DDMediaLogs.cpp:412:25: error: declaration of variable 'json' with deduced type 'JSONStringWriteFunc' requires an initializer
[task 2022-08-17T02:43:03.875Z] 02:43:03     INFO -      JSONStringWriteFunc json;
[task 2022-08-17T02:43:03.876Z] 02:43:03     INFO -                          ^
[task 2022-08-17T02:43:03.876Z] 02:43:03     INFO -  1 error generated.
[task 2022-08-17T02:43:03.876Z] 02:43:03    ERROR -  gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:669: Unified_cpp_dom_media_doctor0.o] Error 1
[task 2022-08-17T02:43:03.876Z] 02:43:03     INFO -  gmake[4]: Leaving directory '/builds/worker/workspace/obj-build/dom/media/doctor'
[task 2022-08-17T02:43:03.877Z] 02:43:03    ERROR -  gmake[3]: *** [/builds/worker/checkouts/gecko/config/recurse.mk:72: dom/media/doctor/target-objects] Error 2
[task 2022-08-17T02:43:03.877Z] 02:43:03     INFO -  gmake[3]: *** Waiting for unfinished jobs....
[task 2022-08-17T02:43:03.889Z] 02:43:03     INFO -  gmake[4]: Entering directory '/builds/worker/workspace/obj-build/dom/html'

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you for the report, sorry for the error.

It looks like some compilers don't like naked types when that type is a template with a default template argument.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4e58c0d264b
JSONWriter::WriteFunc() returns a reference - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/fd1b516d1b4d
JSONWriter may optionally not own its writer - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/5bffb155bc3d
Use common JSONWriteFuncs when writing to a string - r=canaltinova,media-playback-reviewers,alwu
https://hg.mozilla.org/integration/autoland/rev/9d3968f5ee73
Make all JSONWriteFunc-derived classes and their overriden methods final - r=canaltinova

Comment 9

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/f4e58c0d264b
https://hg.mozilla.org/mozilla-central/rev/fd1b516d1b4d
https://hg.mozilla.org/mozilla-central/rev/5bffb155bc3d
https://hg.mozilla.org/mozilla-central/rev/9d3968f5ee73

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3e5982184b52
Port bug 1784812 - JSONWriter refactoring and de-duplication. rs=bustage-fix

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you Geoff for the follow-up.

Comment 12

[Geoff Lankow (:darktrojan)]

There's another one coming, because I forgot that some of our code doesn't run on my platform!