1351385 - Lazily load JSON viewer

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, enhancement)

Description

[Andrew McCreight [:mccr8]]

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 1

[Andrew McCreight [:mccr8]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c690cf3696a5218cbad72565fae4130e63020745

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351385, part 1 - Rename JSON viewer things in preparation for a move to another file.

Review commit: https://reviewboard.mozilla.org/r/124734/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124734/

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351385, part 2 - Lazily load JsonViewService.

This avoids triggering the devtools loading system just to register a
factory.

Review commit: https://reviewboard.mozilla.org/r/124736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124736/

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351385, part 3 - Rename stuff for the JSON sniffer.

This is in preparation for moving it to converter-observer.js.

Review commit: https://reviewboard.mozilla.org/r/124738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124738/

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351385, part 4 - Move the JSON sniffer out of its own file.

The sniffer is always run. By moving it out of its own file we can
avoid importing the devtools loader system at startup, which uses a
number of compartments.

Review commit: https://reviewboard.mozilla.org/r/124740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124740/

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8852545 [details]
Bug 1351385, part 3 - Rename stuff for the JSON sniffer.

https://reviewboard.mozilla.org/r/124738/#review127660

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8852544 [details]
Bug 1351385, part 2 - Lazily load JsonViewService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124736/diff/1-2/

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 8852545 [details]
Bug 1351385, part 3 - Rename stuff for the JSON sniffer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124738/diff/1-2/

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 8852546 [details]
Bug 1351385, part 4 - Move the JSON sniffer out of its own file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124740/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c178900a674f
https://hg.mozilla.org/mozilla-central/rev/981ae021668e
https://hg.mozilla.org/mozilla-central/rev/70dea85b00e3
https://hg.mozilla.org/mozilla-central/rev/0887857a8903