Closed Bug 1396634 Opened 7 years ago Closed 7 years ago

TabActor.webextensionsContentScriptGlobals is slow as it loads ExtensionContent.jsm

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: ochameau, Assigned: rpl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

http://searchfox.org/mozilla-central/source/devtools/server/actors/tab.js#338-345
This is used to fetch all the content script globals related to the targeted document.
The main reason why it is slow is that it loads ExtensionContent.jsm.
  https://perfht.ml/2eyjTfv

It takes a significant time of JS processing in the first chunk of actor's setup in the content process.
As briefly discussed over IRC, by looking at the GeckoProfiles report linked in Comment 0 it seems that the lazy module loading cost (related to the ExtensionContent.jsm module) is what is slowing the things down.

I'm adding a needinfo assigned to me: I'm going to investigate it a bit more deeply and put together a draft patch to avoid the module loading and collect a GeckoProfile report from it, so that we can compare it with the report linked in Comment 0.
Flags: needinfo?(lgreco)
The attached patch uses an alternative approach to retrieve the content script globals for a given window, instead of importing ExtensionContent.jsm using a lazy getter (which also import the module when there are no content scripts around), it uses `Services.obs` to ask for the existent content script globals to an observer registered from the DocumentManager defined in the ExtensionContent.jsm module (which is going to exist only if there are WebExtensions installed).

The following is the GeckoProfiler report collected using the following STR with the attached patch applied:

- Firefox opened with only one single tab
- "data:text/html,<h1>Test Page</h1>" loaded in the single opened tab
- started the GeckoProfiler
- opened the devtools toolbox on the tab
- stopped the profiling

https://perfht.ml/2wDR35n
Flags: needinfo?(lgreco)
Comment on attachment 8904555 [details]
Bug 1396634 - Speed up the content scripts globals retrieval from the tab actor.

https://reviewboard.mozilla.org/r/176410/#review181748

::: devtools/server/actors/tab.js:353
(Diff revision 1)
> +      // Retrieve the existent content script globals for the current window
> +      // (an observer subscribed by the ExtensionContent.jsm will provide the
> +      // existent content script globals for the given window).
> +      Services.obs.notifyObservers(subject, "webext-getContentScriptGlobalsFor");
> +
> +      return results;

Oh I never saw such pattern with notifyObservers,
that's interesting.
But now that I see that, I'm wondering if it would be better (less cryptic) to do:
if (this.window && Cu.isModuleLoaded(ExtensionContent.jsm))
  return ExtensionContent.getContentScriptGlobals(this.window);
(and comment about why we care to not load it)
Attachment #8904555 - Flags: review+
Comment on attachment 8904555 [details]
Bug 1396634 - Speed up the content scripts globals retrieval from the tab actor.

https://reviewboard.mozilla.org/r/176410/#review181748

> Oh I never saw such pattern with notifyObservers,
> that's interesting.
> But now that I see that, I'm wondering if it would be better (less cryptic) to do:
> if (this.window && Cu.isModuleLoaded(ExtensionContent.jsm))
>   return ExtensionContent.getContentScriptGlobals(this.window);
> (and comment about why we care to not load it)

I definitely agree, using `Cu.isModuleLoaded` would be much more clear, and it will definitely provide the same performance win if the module has never been loaded.

I'm totally ok with this version (which is also simpler and it doesn't require any inline comment to explain what is actually happening), but I'm also wondering if the lazy getter would still slow down the actor initialization when a WebExtension is installed (the scenario that I'm thinking of is related to a WebExtensions system addons, like screenshot, which will be always installed), my guess is that it is not going to slow it down as much as it does when the module has not being loaded and that we can go on with the less cryptic strategy.

Anyway, I'm going to collect a GeckoProfiler report using a version of this patch with `Cu.isModuleLoaded` and a single webextension installed with a single content script, just to get an idea of it.
Comment on attachment 8904555 [details]
Bug 1396634 - Speed up the content scripts globals retrieval from the tab actor.

https://reviewboard.mozilla.org/r/176410/#review181748

> I definitely agree, using `Cu.isModuleLoaded` would be much more clear, and it will definitely provide the same performance win if the module has never been loaded.
> 
> I'm totally ok with this version (which is also simpler and it doesn't require any inline comment to explain what is actually happening), but I'm also wondering if the lazy getter would still slow down the actor initialization when a WebExtension is installed (the scenario that I'm thinking of is related to a WebExtensions system addons, like screenshot, which will be always installed), my guess is that it is not going to slow it down as much as it does when the module has not being loaded and that we can go on with the less cryptic strategy.
> 
> Anyway, I'm going to collect a GeckoProfiler report using a version of this patch with `Cu.isModuleLoaded` and a single webextension installed with a single content script, just to get an idea of it.

I've collected some additional GeckoProfiler reports using a version of this patch which uses Cu.isModuleLoaded as suggested, and (as expected) it seems that we are able to gain basically the same performance boost with this version, and given that it is also much more readable I think that we can definitely proceed with this version.
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/da77a0d38379
Speed up the content scripts globals retrieval from the tab actor. r=ochameau
https://hg.mozilla.org/mozilla-central/rev/da77a0d38379
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.