Closed Bug 1800641 Opened 2 years ago Closed 4 months ago

Move value-like fields of ScriptLoadRequest into LoadedScript

Tracking

()

Status:

RESOLVED FIXED

Milestone:

123 Branch

Tracking Flags:

Tracking

Status

firefox123

---

fixed

People

(Reporter: nbp, Assigned: nbp, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: perf-alert)

Attachments

(12 files, 1 obsolete file)

Bug 1800641 - Part 12: Move some ScriptLoadRequest fields into LoadedScript. r=nbp!,yulia! 1 year ago Nicolas B. Pierron [:nbp] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1800641 - Part 1: Move ScriptFetchOptions into its own file. r?nbp! 4 months ago Tooru Fujisawa [:arai] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1800641 - Part 2: Add ScriptLoadRequest::Bytecode and let JSExecutionContext::Decode receive JS::TranscodeRange. r?nbp! 4 months ago Tooru Fujisawa [:arai] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1800641 - Part 3: Add ScriptLoadRequest::SRIAndBytecode. r?nbp! 4 months ago Tooru Fujisawa [:arai] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1800641 - Part 4: Add ScriptLoadRequest::{Get,Set}SRILength. r?nbp! 4 months ago Tooru Fujisawa [:arai] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1800641 - Part 5: Add ScriptLoadRequest::DropBytecode. r?nbp! 4 months ago Tooru Fujisawa [:arai] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1800641 - Part 6: Add ScriptLoadRequest::{,Set}ReceivedScriptTextLength. r?nbp! 4 months ago Tooru Fujisawa [:arai] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1800641 - Part 7: Add ScriptLoadRequest::State::{CheckingCache,PendingFetchingError}. r?nbp! 4 months ago Tooru Fujisawa [:arai] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1800641 - Part 8: Add LoadContextBase* parameter to ScriptLoadRequest methods which is going to be moved to LoadedScriptDelegate. r?nbp! 4 months ago Tooru Fujisawa [:arai] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1800641 - Part 9: Add LoadedScript::mURI and call SetBaseURL separately. r?nbp! 4 months ago Tooru Fujisawa [:arai] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1800641 - Part 10: Add ScriptLoadRequest::mLoadedScript. r?nbp! 4 months ago Tooru Fujisawa [:arai] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1800641 - Part 11: Allocate script in ScriptLoadRequest::NoCacheEntryFound. r?nbp! 4 months ago Tooru Fujisawa [:arai] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1800641 - Part 12: Move some ScriptLoadRequest fields into LoadedScript. r=nbp!,yulia! 4 months ago Tooru Fujisawa [:arai] 48 bytes, text/x-phabricator-request		Details \| Review

Nicolas B. Pierron [:nbp]

Assignee

Description

•

2 years ago

ScriptLoadRequest is meant to handle loading the content out of a channel, verifying the integrity and saving the stored content into a value which can then be used without the need for the ScriptLoadRequest to be alive.

At the moment, we never had the need for keeping the value alive as the JavaScript engine already takes the ownership of the source, likely by cloning it. However, if we want to make these bytes shareable, we need a proper Value type for these to be stored.

The goal of this bug is to move some of the fields of ScriptLoadRequest into LoadedScript, such that having a caching mechanism becomes meaningful.

Nicolas B. Pierron [:nbp]

Assignee

Comment 1

•

1 year ago

Attached file Bug 1800641 - Part 12: Move some ScriptLoadRequest fields into LoadedScript. r=nbp!,yulia! (obsolete) — Details

We are looking into caching loaded script in memory. To do so we need something
to cache.

At the moment, the ScriptLoadRequest structure hold all the fields which are
loaded, and used before executing JavaScript code. Then, the ScriptLoadRequest
is not guaranteed to out-live the first execution.

Therefore, we have to move fields out of the ScriptLoadRequest such that they
can later be used by any caching mechanism. The LoadedScript is the closest
existing structure which exists which fit the description.

This patch moves fields out of the ScriptLoadRequest into the LoadedScript,
which already has a LoadedScript field.

The LoadedScript field is initialized sooner, when the ScriptLoadRequest is
created, to be subsituted later by a real cache implementation. At the moment
the function ScriptLoadRequest::NoCacheEntryFound is used as a placeholder to
change the state of the ScriptLoadRequest from CheckingCache to Fetching.
Existing initializations are replaced by assertions to fail in debug build if
the current patch does not reproduce the expected state properly.

The LoadedScript get fields such as the source text, the text length, the
bytecode buffer (which also contains SRI), and the offset at which the bytecode
starts within the bytecode buffer. As these fields are no longer reachable by
name, multiple accessors are added to work-around the issue. Using this as an
opportunity to add extra assertions as part of these accessors.

A new class named LoadedScriptDelegate is added to re-add, by inheritance, all
the accessors which used to be part of ScriptLoadRequest as methods which are
delegating to the field which is holding the LoadedScript. This class is using
templates to avoid virtual inheritance which might hinder inlining, especially
since ScriptLoadRequest cannot be made final, as ModuleLoadRequest extends
it.

The ScriptFetchOptions structure is moved to its own file to solve C++ include
issues.

BugBot [:suhaib / :marco/ :calixte]

Comment 2

•

5 months ago

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:nbp, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Flags: needinfo?(ystartsev)

Flags: needinfo?(nicolas.b.pierron)

Nicolas B. Pierron [:nbp]

Assignee

Comment 3

•

5 months ago

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #2)

:nbp, could you have a look please?

There are still some failures caused by the rebase of the patch which requires calling NoCacheEntryFound to initialize the LoadedScript sooner, or to delay the checks which are testing properties of the LoadedScript such as whether it stores text or bytecode after the NoCacheEntryFound call.

https://treeherder.mozilla.org/jobs?repo=try&collapsedPushes=1234692%2C1235392%2C1235350&revision=4687b060db88645f74907f6f9a29376ec5996a43

Flags: needinfo?(ystartsev) → needinfo?(arai.unmht)

Phabricator Automation

Updated

•

4 months ago

Attachment #9306268 - Attachment description: Bug 1800641 - Move some ScriptLoadRequest fields into LoadedScript. → Bug 1800641 - Move some ScriptLoadRequest fields into LoadedScript. r=arai!,yulia!

Pulsebot

Comment 4

•

4 months ago

Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/f69f493b2f3c
Move some ScriptLoadRequest fields into LoadedScript. r=yulia

Cosmin Sabou [:CosminS]

Comment 5

•

4 months ago

Backed out for causing non-unified build bustages on LoadedScript.cpp.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=f69f493b2f3cde5ef4b46b9675cfd4708287666e&selectedTaskRun=YznMT2ymTMGK_GGH3DFTkQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=441334986&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/5c45743d053bcdfa7ac4b56193c1a5e30beb7d5f

Pulsebot

Comment 6

•

4 months ago

Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/4d6294424819
Move some ScriptLoadRequest fields into LoadedScript. r=yulia

Tooru Fujisawa [:arai]

Updated

•

4 months ago

Flags: needinfo?(arai.unmht)

Serban Stanca [:SerbanS]

Comment 7

•

4 months ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/4d6294424819

Status: ASSIGNED → RESOLVED

Closed: 4 months ago

status-firefox123: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → 123 Branch

Andra Esanu (needinfo me)

Updated

•

4 months ago

Regressions: 1872347

Noemi Erli[:noemi_erli]

Comment 8

•

4 months ago

Backed out changeset 4d6294424819 (Bug 1800641) for causing Bug 1872347
https://hg.mozilla.org/integration/autoland/rev/4d00abb11b573ff386a9f02816ff81f2030e6dfe

Status: RESOLVED → REOPENED

status-firefox123: fixed → ---

Resolution: FIXED → ---

Target Milestone: 123 Branch → ---

Noemi Erli[:noemi_erli]

Updated

•

4 months ago

Regressions: 1871871

Sandor Molnar[:smolnar]

Comment 9

•

4 months ago

Backout merge to central
https://hg.mozilla.org/mozilla-central/rev/4d00abb11b57

Andra Esanu (needinfo me)

Updated

•

4 months ago

Regressions: 1872990

Alex Finder

Comment 10

•

4 months ago

(In reply to Pulsebot from comment #4)

Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/f69f493b2f3c
Move some ScriptLoadRequest fields into LoadedScript. r=yulia

== Change summary for alert #40796 (as of Thu, 28 Dec 2023 13:48:29 GMT) ==

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
22%	instagram loadtime	macosx1015-64-shippable-qr	fission warm webrender	395.18 -> 482.46	Before/After
21%	instagram ContentfulSpeedIndex	linux1804-64-shippable-qr	fission warm webrender	432.98 -> 522.02	Before/After
20%	instagram SpeedIndex	macosx1015-64-shippable-qr	fission warm webrender	413.99 -> 498.17	Before/After
20%	instagram PerceptualSpeedIndex	macosx1015-64-shippable-qr	fission warm webrender	305.48 -> 366.36	Before/After
20%	instagram loadtime	linux1804-64-shippable-qr	fission warm webrender	540.32 -> 646.66	Before/After
19%	instagram largestContentfulPaint	macosx1015-64-shippable-qr	fission warm webrender	497.93 -> 592.48	Before/After
19%	yahoo-mail fcp	macosx1015-64-shippable-qr	bytecode-cached fission warm webrender	123.31 -> 146.45	Before/After
19%	instagram SpeedIndex	linux1804-64-shippable-qr	fission warm webrender	570.22 -> 676.15	Before/After
17%	instagram PerceptualSpeedIndex	windows10-64-shippable-qr	fission warm webrender	311.08 -> 363.30	Before/After
16%	instagram PerceptualSpeedIndex	linux1804-64-shippable-qr	fission warm webrender	367.70 -> 426.20	Before/After
...	...	...	...	...	...
3%	google-slides LastVisualChange	linux1804-64-shippable-qr	bytecode-cached fission warm webrender	1,564.61 -> 1,611.62	Before/After
3%	linkedin PerceptualSpeedIndex	linux1804-64-shippable-qr	fission warm webrender	708.89 -> 728.87	Before/After
3%	linkedin LastVisualChange	windows10-64-shippable-qr	fission warm webrender	1,849.32 -> 1,900.15	Before/After
3%	linkedin ContentfulSpeedIndex	linux1804-64-shippable-qr	fission warm webrender	1,049.78 -> 1,077.56	Before/After
2%	cnn LastVisualChange	macosx1015-64-shippable-qr	bytecode-cached fission warm webrender	1,033.11 -> 1,055.82

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=40796

Keywords: perf-alert

Acasandrei Beatrice (needinfo me)

Comment 11

•

4 months ago

(In reply to Noemi Erli[:noemi_erli] from comment #8)

Backed out changeset 4d6294424819 (Bug 1800641) for causing Bug 1872347
https://hg.mozilla.org/integration/autoland/rev/4d00abb11b573ff386a9f02816ff81f2030e6dfe

== Change summary for alert #40817 (as of Mon, 01 Jan 2024 22:00:25 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
19%	yahoo-mail fcp	macosx1015-64-shippable-qr	bytecode-cached fission warm webrender	145.86 -> 118.24	Before/After
18%	instagram loadtime	macosx1015-64-shippable-qr	fission warm webrender	481.80 -> 392.88	Before/After
18%	instagram PerceptualSpeedIndex	macosx1015-64-shippable-qr	fission warm webrender	366.85 -> 302.42	Before/After
17%	instagram loadtime	linux1804-64-shippable-qr	fission warm webrender	649.21 -> 537.55	Before/After
17%	instagram ContentfulSpeedIndex	windows10-64-shippable-qr	fission warm webrender	516.58 -> 427.76	Before/After
...	...	...	...	...	...
2%	cnn LastVisualChange	linux1804-64-shippable-qr	bytecode-cached fission warm webrender	1,299.18 -> 1,272.94	Before/After

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=40817

Tooru Fujisawa [:arai]

•

4 months ago

Attachment #9306268 - Attachment description: Bug 1800641 - Move some ScriptLoadRequest fields into LoadedScript. r=arai!,yulia! → Bug 1800641 - Part 12: Move some ScriptLoadRequest fields into LoadedScript. r=nbp!,yulia!

Tooru Fujisawa [:arai]

Comment 23

•

4 months ago

Attached file Bug 1800641 - Part 12: Move some ScriptLoadRequest fields into LoadedScript. r=nbp!,yulia! — Details

We are looking into caching loaded script in memory. To do so we need something
to cache.

This patch moves fields out of the ScriptLoadRequest into the LoadedScript,
which already has a LoadedScript field.

The ScriptFetchOptions structure is moved to its own file to solve C++ include
issues.

Phabricator Automation

Updated

•

4 months ago

Attachment #9306268 - Attachment is obsolete: true

Pulsebot

Comment 24

•

4 months ago

Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/9c47bdaac5b4
Part 1: Move ScriptFetchOptions into its own file. r=nbp
https://hg.mozilla.org/integration/autoland/rev/11e4b8d97eac
Part 2: Add ScriptLoadRequest::Bytecode and let JSExecutionContext::Decode receive JS::TranscodeRange. r=nbp
https://hg.mozilla.org/integration/autoland/rev/ec44e74c3181
Part 3: Add ScriptLoadRequest::SRIAndBytecode. r=nbp
https://hg.mozilla.org/integration/autoland/rev/023a3b57d2cf
Part 4: Add ScriptLoadRequest::{Get,Set}SRILength. r=nbp
https://hg.mozilla.org/integration/autoland/rev/c935fb160052
Part 5: Add ScriptLoadRequest::DropBytecode. r=nbp
https://hg.mozilla.org/integration/autoland/rev/7a5aed81a16d
Part 6: Add ScriptLoadRequest::{,Set}ReceivedScriptTextLength. r=nbp
https://hg.mozilla.org/integration/autoland/rev/512ad299e7e1
Part 7: Add ScriptLoadRequest::State::{CheckingCache,PendingFetchingError}. r=nbp
https://hg.mozilla.org/integration/autoland/rev/015accaf8958
Part 8: Add LoadContextBase* parameter to ScriptLoadRequest methods which is going to be moved to LoadedScriptDelegate. r=nbp
https://hg.mozilla.org/integration/autoland/rev/d3ad88cced28
Part 9: Add LoadedScript::mURI and call SetBaseURL separately. r=nbp
https://hg.mozilla.org/integration/autoland/rev/ccd3c1ac60a5
Part 10: Add ScriptLoadRequest::mLoadedScript. r=nbp
https://hg.mozilla.org/integration/autoland/rev/d274eaca62ce
Part 11: Allocate script in ScriptLoadRequest::NoCacheEntryFound. r=nbp
https://hg.mozilla.org/integration/autoland/rev/f30cc385a875
Part 12: Move some ScriptLoadRequest fields into LoadedScript. r=nbp

Iulian Moraru

Comment 25

•

4 months ago

bugherder

Status: REOPENED → RESOLVED

Closed: 4 months ago → 4 months ago

status-firefox123: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → 123 Branch

Acasandrei Beatrice (needinfo me)

Comment 26

•

3 months ago

(In reply to Pulsebot from comment #24)

Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/9c47bdaac5b4
Part 1: Move ScriptFetchOptions into its own file. r=nbp
https://hg.mozilla.org/integration/autoland/rev/11e4b8d97eac
Part 2: Add ScriptLoadRequest::Bytecode and let JSExecutionContext::Decode
receive JS::TranscodeRange. r=nbp
https://hg.mozilla.org/integration/autoland/rev/ec44e74c3181
Part 3: Add ScriptLoadRequest::SRIAndBytecode. r=nbp
https://hg.mozilla.org/integration/autoland/rev/023a3b57d2cf
Part 4: Add ScriptLoadRequest::{Get,Set}SRILength. r=nbp
https://hg.mozilla.org/integration/autoland/rev/c935fb160052
Part 5: Add ScriptLoadRequest::DropBytecode. r=nbp
https://hg.mozilla.org/integration/autoland/rev/7a5aed81a16d
Part 6: Add ScriptLoadRequest::{,Set}ReceivedScriptTextLength. r=nbp
https://hg.mozilla.org/integration/autoland/rev/512ad299e7e1
Part 7: Add ScriptLoadRequest::State::{CheckingCache,PendingFetchingError}.
r=nbp
https://hg.mozilla.org/integration/autoland/rev/015accaf8958
Part 8: Add LoadContextBase* parameter to ScriptLoadRequest methods which is
going to be moved to LoadedScriptDelegate. r=nbp
https://hg.mozilla.org/integration/autoland/rev/d3ad88cced28
Part 9: Add LoadedScript::mURI and call SetBaseURL separately. r=nbp
https://hg.mozilla.org/integration/autoland/rev/ccd3c1ac60a5
Part 10: Add ScriptLoadRequest::mLoadedScript. r=nbp
https://hg.mozilla.org/integration/autoland/rev/d274eaca62ce
Part 11: Allocate script in ScriptLoadRequest::NoCacheEntryFound. r=nbp
https://hg.mozilla.org/integration/autoland/rev/f30cc385a875
Part 12: Move some ScriptLoadRequest fields into LoadedScript. r=nbp

== Change summary for alert #40933 (as of Tue, 09 Jan 2024 18:19:03 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
5%	fandom loadtime	linux1804-64-shippable-qr	cold fission webrender	1,356.66 -> 1,290.54	Before/After

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=40933

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