Closed Bug 1474155 Opened 3 years ago Closed 3 years ago

Lazify more of browser-content.js and content.js

Categories

(Toolkit :: General, enhancement, P1)

Product:
Toolkit
Component:
General
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: [overhead:30K])

Attachments

(5 files, 4 obsolete files)

Bug 1474155: Part 5 - Null out frame script singleton init methods after they're called.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1474155: Part 1 - Move PopupBlocking to a separate JSM.
59 bytes, text/x-review-board-request
Details
Bug 1474155: Part 2 - Move AutoCompletePopup to a separate JSM.
59 bytes, text/x-review-board-request
Details
Bug 1474155: Part 4 - Move ClickEventHandler to a separate JSM.
59 bytes, text/x-review-board-request
Details
Bug 1474155: Part 3 - Move WebChannel message listeners to a separate JSM.
59 bytes, text/x-review-board-request
mconley
: review+
Details 
There's still quite a lot in browser-content.js and content.js that will never need to be loaded in most iframe documents, and/or will not need to be loaded until the documents are interacted with.

There will need to be several more bugs like this... but let's just keep chipping away.
Whiteboard: [overhead:60K] → [overhead:30K]
Comment on attachment 8990569 [details]
Bug 1474155: Part 1 - Move PopupBlocking to a separate JSM.

https://reviewboard.mozilla.org/r/255632/#review262860

Looks good. Thanks for copying PopupBlocking.jsm from browser-content.js to save the history!

::: toolkit/modules/PopupBlocking.jsm:10
(Diff revision 1)
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");

Unless I'm mistaken, I don't think either of these are used.
Attachment #8990569 - Flags: review?(mconley) → review+
Comment on attachment 8990570 [details]
Bug 1474155: Part 2 - Move AutoCompletePopup to a separate JSM.

https://reviewboard.mozilla.org/r/255634/#review262866

Thanks!

::: toolkit/modules/AutoCompletePopupContent.jsm:10
(Diff revision 1)
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");

Neither of these seem to be used here either.
Attachment #8990570 - Flags: review?(mconley) → review+
Comment on attachment 8990571 [details]
Bug 1474155: Part 3 - Move WebChannel message listeners to a separate JSM.

https://reviewboard.mozilla.org/r/255636/#review262872

r-'ing because I don't think the WebChannelMessageToContent thing is working (at least by inspection). Or am I misunderstanding something here?

::: toolkit/modules/WebChannelContent.jsm:14
(Diff revision 1)
> +
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  
> -ChromeUtils.defineModuleGetter(this, "AutoCompletePopup",
> -  "resource://gre/modules/AutoCompletePopupContent.jsm");
> +function getMessageManager(event) {
> +  let window = ChromeUtils.getGlobalForObject(event.target);

TIL about getGlobalForObject. Handy!

::: toolkit/modules/WebChannelContent.jsm:15
(Diff revision 1)
> -ChromeUtils.defineModuleGetter(this, "AutoScrollController",
> -  "resource://gre/modules/AutoScrollController.jsm");
> +  return window.document.docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +  .getInterface(Ci.nsIContentFrameMessageManager);

From what I've seen, I think this formatting is more idiomatic for long incantations like this:

```js

return window.document
             .docShell
             .QueryInterface(Ci.nsIInterfaceRequestor)
             .getInterface(Ci.nsIContentFrameMessageManager);
             
```

::: toolkit/modules/WebChannelContent.jsm:34
(Diff revision 1)
>  
> -  init() {
> -    addEventListener("WebChannelMessageToChrome", e => {
> -      this._onMessageToChrome(e);
> -    }, true, true);
> +  handleEvent(event) {
> +    switch (event.type) {
> +      case "WebChannelMessageToChrome":
> +        return this._onMessageToChrome(event);
> +      case "WebChannelMessageToContent":

I think I'm missing something here... WebChannelMessageToContent is a message from the message manager, and is only an event, I think, for the page that's being communicated to...

So this should be in a receiveMessage unless there's some message<->event conversion voodoo going on that I'm not seeing.
Attachment #8990571 - Flags: review?(mconley) → review-
Comment on attachment 8990571 [details]
Bug 1474155: Part 3 - Move WebChannel message listeners to a separate JSM.

https://reviewboard.mozilla.org/r/255636/#review262872

> I think I'm missing something here... WebChannelMessageToContent is a message from the message manager, and is only an event, I think, for the page that's being communicated to...
> 
> So this should be in a receiveMessage unless there's some message<->event conversion voodoo going on that I'm not seeing.

Oh. Yes. I knew something seemed wrong there... Whoever had the idea to call that parameter `e` ...
Comment on attachment 8990572 [details]
Bug 1474155: Part 4 - Move ClickEventHandler to a separate JSM.

https://reviewboard.mozilla.org/r/255638/#review262878

This looks okay - but I think you'll want to change the ClickEventHandler to a lazy proxy.

::: browser/base/content/content.js:37
(Diff revision 1)
>  
>  XPCOMUtils.defineLazyProxy(this, "contextMenu", () => {
>    return new ContextMenu(global);
>  });
>  
> +XPCOMUtils.defineLazyGetter(this, "ClickEventHandler", () => {

Did this mean to be a lazy proxy? Because I believe the adding as a system event listener is going to cause this JSM to load.
Attachment #8990572 - Flags: review?(mconley) → review+
Comment on attachment 8990573 [details]
Bug 1474155: Part 5 - Null out frame script singleton init methods after they're called.

https://reviewboard.mozilla.org/r/255640/#review262838

As mentioned in IRC, this is a pretty surprising discovery. This feels like the sort of thing we can easily backslide on because it's not immediately obvious. I'll make sure to call it out at the next bi-weekly Desktop meeting, but we should probably see if there's a way we can either do this automatically, or add some linting to enforce this by machine.

::: commit-message-40252:3
(Diff revision 1)
> +Bug 1474155: Part 5 - Null out frame script singleton init methods after they're called. r?mconley
> +
> +Believe it or not, the memory these references hold alive is significantl

Nit - typo: "significant1" -> "significant!"
Attachment #8990573 - Flags: review?(mconley) → review+
Comment on attachment 8990572 [details]
Bug 1474155: Part 4 - Move ClickEventHandler to a separate JSM.

https://reviewboard.mozilla.org/r/255638/#review262878

> Did this mean to be a lazy proxy? Because I believe the adding as a system event listener is going to cause this JSM to load.

Yeah. Good catch.
Comment on attachment 8990573 [details]
Bug 1474155: Part 5 - Null out frame script singleton init methods after they're called.

https://reviewboard.mozilla.org/r/255640/#review262838

> Nit - typo: "significant1" -> "significant!"

It's an ell, not a one :) It was meant to be a '.'
Attachment #8990569 - Attachment is obsolete: true
Attachment #8990570 - Attachment is obsolete: true
Attachment #8990571 - Attachment is obsolete: true
Attachment #8990572 - Attachment is obsolete: true
I actually tracked down and ran the WebChannel tests this time :) That was the only thing I didn't manage to properly test the first time around.
Priority: -- → P1
(In reply to Kris Maglione [:kmag] from comment #13)
> Comment on attachment 8990573 [details]
> Bug 1474155: Part 5 - Null out frame script singleton init methods after
> they're called.

Can you explain why we would null out the init functions? Is it because we expect that they would only get called once? If so, are there other methods that would exhibit this same behavior? Could we do something to annotate these as self-destructing methods? It also seems like we could write a test that tries to call these self-destructing methods two times and expects no exception in the first call and a TypeError on the second call.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> (In reply to Kris Maglione [:kmag] from comment #13)
> > Comment on attachment 8990573 [details]
> > Bug 1474155: Part 5 - Null out frame script singleton init methods after
> > they're called.
> 
> Can you explain why we would null out the init functions? Is it because we
> expect that they would only get called once? If so, are there other methods
> that would exhibit this same behavior? Could we do something to annotate
> these as self-destructing methods? It also seems like we could write a test
> that tries to call these self-destructing methods two times and expects no
> exception in the first call and a TypeError on the second call.

It's because we know they're only called once, and keeping a reference to them means they continue to consume memory.

I'm not sure what the point of a test would be.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Could we do something to annotate
> these as self-destructing methods? It also seems like we could write a test
> that tries to call these self-destructing methods two times and expects no
> exception in the first call and a TypeError on the second call.

Perhaps this is something we can do with linting. These types of functions tend to be named init and uninit, so assuming they're only ever supposed to be called once per instance, we might be able to add some ESLint foo to check for deletions somewhere in the function (and we opt-out the odd cases where init() or uninit() might be called multiple times).

Unless I'm very mistaken, I suspect we won't be able to get ESLint to detect the single-use-function case automatically without leaning on the naming conventions. :/
(In reply to Kris Maglione [:kmag] from comment #17)
> I'm not sure what the point of a test would be.

The purpose of the test would be to ensure that the `this.init = null` line doesn't get removed without knowledge of the side-effects.
Something kats suggested is to use something like JS coverage to determine in our debug builds when methods on objects in framescripts are called once and only once.
(In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from comment #20)
> Something kats suggested is to use something like JS coverage to determine
> in our debug builds when methods on objects in framescripts are called once
> and only once.

That might be a good idea long term.

In the short term, it would probably be enough to just add an ESLint rule to check that any init() methods in singleton objects have `this.init = null` as their last line.

Well, actually, in the long term I'm hoping to get rid of frame scripts entirely, so...
Oh, did I screw up pushing my updated patches?
Attachment #8992540 - Flags: review?(mconley)
Attachment #8992541 - Flags: review?(mconley)
Attachment #8992542 - Flags: review?(mconley)
(Cleared review? flags on things that already landed)
Comment on attachment 8992543 [details]
Bug 1474155: Part 3 - Move WebChannel message listeners to a separate JSM.

https://reviewboard.mozilla.org/r/257410/#review264504

Looks good, thanks!

::: toolkit/modules/WebChannelContent.jsm:81
(Diff revision 1)
> -// This should be kept in sync with /browser/base/content.js.
> +  // This should be kept in sync with /browser/base/content.js.
> -// Add message listener for "WebChannelMessageToContent" messages from chrome scripts.
> +  // Add message listener for "WebChannelMessageToContent" messages from chrome scripts.

Nit: I don't think these comments make much sense anymore. Probably more confusing than anything, and can be cut.
Attachment #8992543 - Flags: review?(mconley) → review+
AWSY perf wins from either this bug or bug 1474131:

== Change summary for alert #14386 (as of Tue, 17 Jul 2018 05:24:57 GMT) ==

Improvements:

  3%  Base Content Resident Unique Memory windows10-64 pgo stylo     16,838,656.00 -> 16,281,941.33
  3%  Base Content Resident Unique Memory windows10-64 opt stylo     16,883,285.33 -> 16,342,698.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14386
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d3fd5f71734c96286e05e9d3ab0b461e97d55e3
Bug 1474155: Part 3 - Move WebChannel message listeners to a separate JSM. r=mconley
You need to log in before you can comment on or make changes to this bug.