1795873 - browser_xpcom_graph_wait.js doesn't detect modules loaded via static import

Status: RESOLVED FIXED

(Toolkit :: Background Tasks, defect)

Description

[Mark Banner (:standard8)]

In attempting to land bug 1792341, we found an issue with browser_xpcom_graph_wait.js - it isn't detecting modules that are loaded via static import. This caused failures because we transitioned some modules to be exclusively loaded via import in the content process, and the test couldn't see them.

It only detects those loaded via ChromeUtils.import/ChromeUtils.importESModule.

There are currently no profiler markers for static imports.

Comment 1

[Mark Banner (:standard8)]

Nick, is there a reason for needing to check the modules at each phase, or could we check the loaded modules after the task has run?

Comment 2

[Mark Banner (:standard8)]

I asked in the Joy of Profiling channel, and they indicated that it would useful if the profiler markers would work for static imports (especially if it also works for "normal" JS code).

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Banner (:standard8) from comment #1)

Nick, is there a reason for needing to check the modules at each phase, or could we check the loaded modules after the task has run?

The rationale is simply to differentiate between stuff loaded by:

• Firefox startup
• the background task harness
• some task that does something reasonable

Only some of those things can be addressed.

Now: it would be acceptable to check the loaded modules after the task has run; that's better than nothing. Generally, this test isn't providing a huge amount of value: it trips up folks doing legitimate work well outside of the background task subsystem, and it hasn't stopped any meaningful regression. But it works and it is nice to know that we are unlikely to regress by loading large amounts of code too early without any warning.

In this particular instance, however, I really think we want profiler markers for static imports (as well as ChromeUtils.import*), since (AFAIK) they have equivalent runtime cost. And when we have those, this task generalizes smoothly. Do you agree?

Comment 4

[Tooru Fujisawa [:arai]]

If we want profiler markers for statically imported modules, it makes sense to implement it here to solve the testcase problem.

Then, about the runtime cost, the static import has slightly different behavior, even if the cost itself is same:

ChromeUtils.import does:

1. load source
2. compile
3. evaluate top-level code

and if there's another ChromeUtils.import call at the top-level code, all 3 steps are done at once in the caller's step 3,
and all profiler marker has the same structure between importer and importee.

Then, ChromeUtils.importESModule does:

1. load source
2. compile
3. recursively load and compile statically imported modules
4. sequentially evaluate all statically imported modules' top-level code
5. evaluate top-level code

So, at least, each statically imported module is handled with 2 separate steps, load+compile and evaluate,
and thus the profiler markers have different structure.

(if there's another ChromeUtils.importESModule call at the top-level code, all 5 steps are done at once in the caller's step 5, and this has the same structure)

Then, in term of implementation, the main part of the step 3 is done by mozJSModuleLoader::LoadSingleModuleScript for each imported module, and we can simply add system-ESM-specific profiler marker for each statically imported module's load+compile (and also top-level module's steps 1-2),
but step 4 is done inside JSAPI InnerModuleEvaluation, and I'm wondering what would be the best way to add profiler marker for each statically imported module's top-level code evaluation.

If non-system-ESM case also needs profiler marker (especially for evaluation part), directly adding profiler marker inside JSAPI would work, for step 4.
(step 3 needs different handling, given system ESM uses synchronous load, but other cases don't)

Also, if we're interested only in load+compile time, we can just add marker for step 3 and let the top-level module's marker represent everything else including step 4.

jonco, can I have your opinion?

Also, is there any demand for profiler markers for static import, in web content use case?

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: difference between ChromeUtils.import and static import

Here's the example of the profile, with the same module graph, between ChromeUtils.import and static import.
(the order of the imported modules might be wrong in the static import case tho)

Comment 6

[Tooru Fujisawa [:arai]]

I'll add profiler marker for load+compile part (mozJSModuleLoader::LoadSingleModuleScript) for each static import.
that will solve the testcase usage as well.

adding profiler marker for each evaluation part is tricky and won't much beneficial compared to the complexity,
thus we'll leave that part.
the total time taken by evaluating all modules will be represented in the profiler anyway, as the time after load+compile.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Bug 1795873 - Part 1: Add profiler marker for ChromeUtils.importESModule static import. r?jonco!

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Bug 1795873 - Part 2: Monitor ChromeUtils.importESModule static import in browser_xpcom_graph_wait.js. r?Standard8!

Depends on D159920

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/292542f833fe
Part 1: Add profiler marker for ChromeUtils.importESModule static import. r=jonco
https://hg.mozilla.org/integration/autoland/rev/8326cc71c2dd
Part 2: Monitor ChromeUtils.importESModule static import in browser_xpcom_graph_wait.js. r=Standard8

Comment 10

[Marian-Vasile Laza]

Backed out 3 changesets (Bug 1795873, Bug 1796539) as req by the dev (arai).
Backout link

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f83df148e36e
Part 1: Add profiler marker for ChromeUtils.importESModule static import. r=jonco
https://hg.mozilla.org/integration/autoland/rev/7bb2d6da6924
Part 2: Monitor ChromeUtils.importESModule static import in browser_xpcom_graph_wait.js. r=Standard8

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/f83df148e36e
https://hg.mozilla.org/mozilla-central/rev/7bb2d6da6924