Closed Bug 1578730 Opened 1 year ago Closed 1 year ago

Remove code-coverage dependence on JSScript::realm() during finalization

Categories

(Core :: JavaScript Engine, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

References

Details

Attachments

(7 files)

Bug 1575350 removed most of the uses of JSScript::realm() during JSScript finalization, but the lcov stuff still using a LCovRealm. This is currently complicating Bug 1568245.

The approach that I am going with it to replace ScriptNameMap with a HashMap<JSScript*, LCovSource*>. This moves the LCovSource allocation earlier, but simplifies what we do in the JSScript::finalizer. It will also stop us from cloning the filename for each inner function while running code coverage.

Depends on: 1581854

Split the GetCodeCoverageSummary API into a variant for a specific realm
vs checking all realms. This restores the original behaviour of the
getLcovInfo testing function to only return info on current realm. This
makes testing OOM behaviour much more predictable.

Avoid creating the ScriptNameMap in the off-thread parse zone. Instead
perform JSScript::initScriptName when merging the parse result to main
thread. This will avoid the need to migrate coverage initialization data
between Realms.

Depends on D46167

Save a little bit of memory when code coverage is not in use. Add a
collectCodeCoverage method to the realm so it can allocate LCovRealm on
demand. Also compute the realm name string from the constructor instead
of in the lookup code (this still happens at same time).

Depends on D46168

Now that LCovRealm is allocated on demand, use a Vector directly instead
of allocating that vector on demand. To ensure pointer stability of
LCovSources, allocated them in the LifoAlloc and only store pointers in
the sources vector. They are still fully owned by the LCovRealm.

Depends on D46169

To prepare for later patches, track writeScript OOMs as a flag inside
LCovSource. The objective is to be able to call LCovSource::writeScript
directly from the JSScript finalizer without needing access to the
current realm.

Depends on D46170

Remove LCovRealm::collectCodeCoverageInfo and directly call
LCovSource::writeScript from JSScript::finalize and others. In later
patches we will replace the script-name map with a LCovSource map.

Depends on D46171

Rename the ScriptNameMap to ScriptLCovMap and associate LCovSource* with
each script rather than a private clone of filename. This removes the
need for JSScript::finalize to need access to JSScript::realm().

Depends on D46172

The overall objective here is to remove the calls to JSScript::realm() from JSScript::finalize. The result of this patch stack is for a JSScript to be associated with an LCovSource* during initialization. During finalization, the coverage counts for a script are accumulated into the LCovSource.

The first patch (Add js::GetCodeCoverageSummaryAll API) fixes an issue where coverage/bug1214548.js would timeout. The issue was that js::GetCodeCoverageSummary was collecting coverage data beyond just the correct realm.

The second patch (Run JSScript::initScriptName on main-thread only) tries to avoid generating any of the coverage tables in the off-thread parse Zone and defers calling initScriptName until we merge the off-thread parse task back to main thread.

The rest of the patches cleanup LCovRealm/LCovSource so that having a LCovSource* is sufficient to finalize a script. I kept it in parts to make it easier to see progression, but I can also squash if that is clearer.

Keywords: leave-open
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7581d5468864
Add js::GetCodeCovergeSummaryAll API. r=nbp,marco
https://hg.mozilla.org/integration/autoland/rev/985febe1e1a7
Run JSScript::initScriptName on main-thread only. r=nbp
https://hg.mozilla.org/integration/autoland/rev/19172ad2dea7
Allocate LCovRealm on first use. r=nbp
https://hg.mozilla.org/integration/autoland/rev/910a125e41dc
Use vector of pointers for LCovRealm::sources. r=nbp
Attachment #9093352 - Attachment description: Bug 1578730 - Call LCovSource::writeScript directly. r?nbp → Bug 1578730 - Move ScriptNameMap to js/src/vm/CodeCoverage files. r?nbp
Attachment #9093351 - Attachment description: Bug 1578730 - Defer LCovSource::writeScript OOM flag. r?nbp → Bug 1578730 - Track script-associated LCovSource instead of filename. r?nbp
Attachment #9093351 - Attachment description: Bug 1578730 - Track script-associated LCovSource instead of filename. r?nbp → Bug 1578730 - Defer LCovSource::writeScript OOM flag. r?nbp
Attachment #9093353 - Attachment description: Bug 1578730 - Track LCovSource per JSScript when code-coverage is enabled. r?nbp → Bug 1578730 - Track script-associated LCovSource instead of filename. r?nbp
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d80982a429f
Defer LCovSource::writeScript OOM flag. r=nbp
https://hg.mozilla.org/integration/autoland/rev/b194fe05c555
Move ScriptNameMap to js/src/vm/CodeCoverage files. r=nbp
https://hg.mozilla.org/integration/autoland/rev/8a7f8a71391f
Track script-associated LCovSource instead of filename. r=nbp
Keywords: leave-open
Regressions: 1583408
You need to log in before you can comment on or make changes to this bug.