Closed Bug 1562272 Opened 5 months ago Closed 5 months ago

Consider moving atoms out of SharedScriptData

Categories

(Core :: JavaScript Engine, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files)

In order to be able to use bytecode directly from XUL or disk caches (for cross-process read-only memory sharing), we need to do something about the GCPtrAtom in SharedScriptData.

My proposed approach is to split SharedScriptData into RuntimeScriptData/SharedScriptData. The atoms and refcount would be moved to RuntimeScriptData and it would point to the SharedScriptData. This would leave SharedScriptData as fully immutable and relocatable.

In order to prevent frozen SharedScriptData from being cleared, I add a HashSet to the Runtime to hold extra references to keep alive. During shutdown, I clear the RuntimeScriptData::ssd_ pointer and drop the references. This lets the current de-duplication mechanism continue to work.

This splits the ref-count off from js::SharedScriptData. The JSScript
points to the RuntimeScriptData which points to the SharedScriptData.

This is precursor work to allowing script data to be shared between
processes.

This results in SharedScriptData having no pointers left and being an
easy candidate for sharing accross procecces.

The XDR and InitFromEmitter functions will be split in follow-up patches
to make the changes easier to follow.

Depends on D36352

Both of these functions are now split into SharedScriptData and
RuntimeScriptData variants.

Depends on D36353

Here is the prototype. On its own, it regresses memory overhead slightly because of the extra pointer from RuntimeScriptData -> SharedScriptData. There is an extra indirection to access SharedScriptData, but code() is now at a fixed offset leaves it at the same number of loads.

There are still some issues with memory accounting in the runtime, but otherwise this passes tests.

Blocks: 1523749
Attachment #9074834 - Attachment description: Bug 1562272 - Add js::RuntimeScriptData type → Bug 1562272 - Add js::RuntimeScriptData type. r?jandem
Attachment #9074835 - Attachment description: Bug 1562272 - Move script atoms from SharedScriptData to RuntimeScriptData → Bug 1562272 - Move script atoms from SharedScriptData to RuntimeScriptData. r?jandem
Attachment #9074836 - Attachment description: Bug 1562272 - Split SharedScriptData::XDR and InitFromElement → Bug 1562272 - Split SharedScriptData::XDR and InitFromElement. r?jandem

The memory reporting and leaks have been fixed. By good fortune, the new breakdown of jemalloc rounding leads to this being a tiny memory win instead of the regression that I expected from the extra pointer. It is now read for review and landing.

This is rebased just after the 70.0 merge. I think this is now in sync with the Baseline interpreter but I'll rerun some more of those tests.

Need a part 4 to rename SharedScriptDataHasher to RuntimeScriptDataHasher, etc.

Keywords: leave-open
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/af6d2ceb1b9d
Add js::RuntimeScriptData type. r=jandem
https://hg.mozilla.org/integration/autoland/rev/c1add3b2d16e
Move script atoms from SharedScriptData to RuntimeScriptData. r=jandem
https://hg.mozilla.org/integration/autoland/rev/83e3a8db101e
Split SharedScriptData::XDR and InitFromElement. r=jandem
Keywords: leave-open
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0860d74cb2ef
Rename js::ScriptDataTable to RuntimeScriptDataTable. r=jandem
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Blocks: 1565556

== Change summary for alert #21889 (as of Mon, 15 Jul 2019 05:30:13 GMT) ==

Improvements:

0.32% Base Content JS linux64-shippable opt 4,039,280.00 -> 4,026,304.00
0.32% Base Content JS linux64-shippable-qr opt 4,039,333.33 -> 4,026,304.00
0.27% Base Content JS macosx1014-64-shippable opt 4,038,234.67 -> 4,027,280.00
0.23% Base Content JS macosx1014-64-shippable opt 4,036,510.67 -> 4,027,280.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=21889

== Change summary for alert #22222 (as of Thu, 01 Aug 2019 04:46:48 GMT) ==

Improvements:

4% tp5o_webext responsiveness linux64-shippable opt e10s stylo 1.78 -> 1.70
3% tp5o_webext responsiveness linux64-shippable opt e10s stylo 1.78 -> 1.72

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=22222

You need to log in before you can comment on or make changes to this bug.