Closed Bug 1351385 Opened 6 years ago Closed 6 years ago

Lazily load JSON viewer

Categories

(DevTools :: JSON Viewer, enhancement)

enhancement
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Whiteboard: [MemShrink])

Attachments

(4 files)

At startup in the content process, we run the JSON view converter-observer (in all release branches). This causes us to import the whole devtools loading infrastructure, which creates 5 different compartments, even if the user never uses the viewer. With 4 content processes, that's about 600kb of memory.

Fixing this has two parts:

1. The factory registration code in converter-child.js needs to be moved into converter-observer.js. This will then load -child.js if an instance is actually created.

2. The code for converter-sniffer.js has to be moved entirely into -observer. The sniffer appears to be run regularly, so we can't avoid loading it.
Comment on attachment 8852543 [details]
Bug 1351385, part 1 - Rename JSON viewer things in preparation for a move to another file.

https://reviewboard.mozilla.org/r/124734/#review127658
Attachment #8852543 - Flags: review?(odvarko) → review+
Comment on attachment 8852544 [details]
Bug 1351385, part 2 - Lazily load JsonViewService.

https://reviewboard.mozilla.org/r/124736/#review127656

::: devtools/client/jsonview/converter-observer.js:45
(Diff revision 1)
> +const JSON_VIEW_CLASS_DESCRIPTION = "JSONView converter";
> +
> +/*
> + * Create instances of the JSON view converter.
> + */
> +const JsonViewFactory = {

Please make a comment about why the factory is here.
Attachment #8852544 - Flags: review?(odvarko) → review+
Comment on attachment 8852545 [details]
Bug 1351385, part 3 - Rename stuff for the JSON sniffer.

https://reviewboard.mozilla.org/r/124738/#review127660
Attachment #8852545 - Flags: review?(odvarko) → review+
Comment on attachment 8852546 [details]
Bug 1351385, part 4 - Move the JSON sniffer out of its own file.

https://reviewboard.mozilla.org/r/124740/#review127662

::: devtools/client/jsonview/converter-observer.js:52
(Diff revision 1)
> + * document types to: 'application/vnd.mozilla.json.view'.
> + *
> + * This internal type is consequently rendered by JSON View component
> + * that represents the JSON through a viewer interface.
> + */
> +function JsonViewSniffer() {}

Please make a comment about why the entire JsonViewSniffer is here.
Attachment #8852546 - Flags: review?(odvarko) → review+
Thanks for splitting the work into more patches, it made the review a lot easier!

It looks good to me, just having more comments in the code about this change would be great.

Honza
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c178900a674f
part 1 - Rename JSON viewer things in preparation for a move to another file. r=Honza
https://hg.mozilla.org/integration/autoland/rev/981ae021668e
part 2 - Lazily load JsonViewService. r=Honza
https://hg.mozilla.org/integration/autoland/rev/70dea85b00e3
part 3 - Rename stuff for the JSON sniffer. r=Honza
https://hg.mozilla.org/integration/autoland/rev/0887857a8903
part 4 - Move the JSON sniffer out of its own file. r=Honza
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.