1649599 - SessionFile.jsm imports OS.File during startup

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, task)

Description

[Beth Rennie [:beth] (she/her)]

According to this startup profile, SessionFile.jsm runs during startup and imports osfile.jsm. We should migrate this to the new IOUtils replacement for osfile.jsm when possible.

Stack:

(root) []
XREMain::XRE_main []
XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) [libxul.so]
XREMain::XRE_mainRun() [libxul.so]
nsObserverService::NotifyObservers final-ui-startup []
nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) [libxul.so]
nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) [libxul.so]
SharedStub [libxul.so]
PrepareAndDispatch [libxul.so]
XPCWrappedJS method call []
nsXPCWrappedJS::CallMethod(unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) [libxul.so]
JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) [libxul.so]
js::RunScript []
BG_observe [resource:///modules/BrowserGlue.jsm:936:36]
BG__beforeUIStartup [resource:///modules/BrowserGlue.jsm:1264:48]
init [resource:///modules/sessionstore/SessionStartup.jsm:119:6]
js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [libxul.so]
Interpret(JSContext*, js::RunState&) [libxul.so]
js::NativeGetExistingProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<js::NativeObject*>, JS::Handle<js::Shape*>, JS::MutableHandle<JS::Value>) [libxul.so]
js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) [libxul.so]
js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [libxul.so]
mozilla::dom::module_getter::ModuleGetter(JSContext*, unsigned int, JS::Value*) [libxul.so]
mozJSComponentLoader::Import(JSContext*, nsTSubstring<char> const&, JS::MutableHandle<JSObject*>, JS::MutableHandle<JSObject*>, bool) [libxul.so]
component loader load module []
mozJSComponentLoader::ObjectForLocation(ComponentLoaderInfo&, nsIFile*, JS::MutableHandle<JSObject*>, JS::MutableHandle<JSScript*>, char**, bool, JS::MutableHandle<JS::Value>) [libxul.so]
js::ExecuteInJSMEnvironment(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>) [libxul.so]
js::ExecuteInJSMEnvironment(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::Handle<JS::StackGCVector<JSObject*, js::TempAllocPolicy> >) [libxul.so]
ExecuteInExtensibleLexicalEnvironment(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>) [libxul.so]
js::RunScript []
> resource:///modules/sessionstore/SessionFile.jsm []
js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::Handle<JS::Value>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) [libxul.so]
Interpret(JSContext*, js::RunState&) [libxul.so]
js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [libxul.so]
ChromeUtils.import []
mozilla::dom::ChromeUtils_Binding::import(JSContext*, unsigned int, JS::Value*) [libxul.so]
> ChromeUtils::Import resource://gre/modules/osfile.jsm []
mozilla::dom::ChromeUtils::Import(mozilla::dom::GlobalObject const&, nsTSubstring<char16_t> const&, mozilla::dom::Optional<JS::Handle<JSObject*> > const&, JS::MutableHandle<JSObject*>, mozilla::ErrorResult&) [libxul.so]
mozJSComponentLoader::Import(JSContext*, nsTSubstring<char> const&, JS::MutableHandle<JSObject*>, JS::MutableHandle<JSObject*>, bool) [libxul.so]
profiler_get_backtrace() [libxul.so]
Registers::SyncPopulate() [libxul.so]

Comment 1

[Michelle Goossens [:masterwayz]]

Attachment: Bug 1649599 - Convert SessionFile.jsm to use IOUtils r=Standard8

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17448bde9d5d
Convert SessionFile.jsm to use IOUtils r=emalysz

Comment 3

[Atila Butkovits]

Backed out for turning Bug 1579683 into permafail.

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

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&searchStr=linux%2Cx64%2Cshippable%2Copt%2Ctalos%2Cperformance%2Ctests%2Ctest-linux64-shippable%2Fopt-talos-realworld-webextensions-e10s%2Crealwebext&revision=17448bde9d5db1cb1be1992541571e36cf6b2509&selectedTaskRun=bWFQlxqeR8auCaM9gPXLvQ.0

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

Comment 4

[Michelle Goossens [:masterwayz]]

I've been looking into this with emalysz.

Need to do more investigative work on this one.

Comment 5

[Michelle Goossens [:masterwayz]]

The patch has been updated with a patch that this depends on, and it this state is green on try for the failing talos test.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/453811f08638
Convert SessionFile.jsm to use IOUtils r=emalysz

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/453811f08638

Comment 8

[Alexandru Michis [:malexandru]]

Backed out changeset 453811f08638 (Bug 1649599) for causing performance regressions seen in Bug 1683885.

Backout link: https://hg.mozilla.org/integration/autoland/rev/9137f77b888fe2418557903392a4301c0f76dfa6

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/9137f77b888f

Comment 10

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Alexandru Michis [:malexandru] from comment #8)

Backed out changeset 453811f08638 (Bug 1649599) for causing performance regressions seen in Bug 1683885.

Backout link: https://hg.mozilla.org/integration/autoland/rev/9137f77b888fe2418557903392a4301c0f76dfa6

== Change summary for alert #28224 (as of Fri, 25 Dec 2020 00:59:54 GMT) ==

Regressions:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
190%	tp5n	nonmain_startup_fileio	windows10-64-shippable-qr	e10s stylo webrender-sw	739,361.50 -> 2,146,090.25
187%	tp5n	nonmain_startup_fileio	windows10-64-shippable	e10s stylo	736,561.25 -> 2,116,439.75
157%	tp5n	nonmain_startup_fileio	windows10-64-shippable-qr	e10s stylo webrender	737,633.92 -> 1,898,813.58
68%	tp5n	main_startup_fileio	windows10-64-shippable	e10s stylo	436,080.58 -> 730,509.25
57%	tp5n	main_startup_fileio	windows10-64-shippable-qr	e10s stylo webrender-sw	518,191.50 -> 813,901.50
55%	tp5n	main_startup_fileio	windows10-64-shippable-qr	e10s stylo webrender	518,103.67 -> 804,000.17
21%	sessionrestore_no_auto_restore	sessionrestore_no_auto_restore	linux64-shippable-qr	e10s stylo webrender-sw	483.00 -> 584.08
17%	sessionrestore_no_auto_restore	sessionrestore_no_auto_restore	linux64-shippable	e10s stylo	495.12 -> 577.75
15%	sessionrestore_no_auto_restore	sessionrestore_no_auto_restore	windows10-64-shippable	e10s stylo	508.92 -> 586.50
15%	sessionrestore_no_auto_restore	sessionrestore_no_auto_restore	windows10-64-shippable-qr	e10s stylo webrender-sw	512.83 -> 590.08
12%	sessionrestore	sessionrestore	windows10-64-shippable	e10s stylo	473.65 -> 532.17
10%	sessionrestore	sessionrestore	windows10-64-shippable-qr	e10s stylo webrender	483.24 -> 533.92
10%	sessionrestore	sessionrestore	windows10-64-shippable-qr	e10s stylo webrender-sw	494.92 -> 545.50
8%	sessionrestore	sessionrestore	linux64-shippable	e10s stylo	485.47 -> 522.42

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
32%	about_newtab_with_snippets	responsiveness	linux64-shippable-qr	e10s stylo webrender	0.25 -> 0.17
12%	startup_about_home_paint	startup_about_home_paint	linux64-shippable	e10s stylo	739.58 -> 652.25
11%	startup_about_home_paint_realworld_webextensions	startup_about_home_paint_realworld_webextensions	linux64-shippable	e10s stylo	769.33 -> 682.17
10%	startup_about_home_paint	startup_about_home_paint	windows10-64-shippable-qr	e10s stylo webrender	751.24 -> 675.92
9%	startup_about_home_paint	startup_about_home_paint	windows10-64-shippable-qr	e10s stylo webrender-sw	725.00 -> 662.00
9%	startup_about_home_paint_realworld_webextensions	startup_about_home_paint_realworld_webextensions	linux64-shippable-qr	e10s stylo webrender	948.00 -> 866.33
8%	startup_about_home_paint	startup_about_home_paint	linux64-shippable-qr	e10s stylo webrender	936.83 -> 861.25
6%	startup_about_home_paint_realworld_webextensions	startup_about_home_paint_realworld_webextensions	windows10-64-shippable	e10s stylo	758.00 -> 713.58

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

Comment 11

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Alexandru Michis [:malexandru] from comment #8)

Backed out changeset 453811f08638 (Bug 1649599) for causing performance regressions seen in Bug 1683885.

Backout link: https://hg.mozilla.org/integration/autoland/rev/9137f77b888fe2418557903392a4301c0f76dfa6

== Change summary for alert #28224 (as of Fri, 25 Dec 2020 00:59:54 GMT) ==

Regressions:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
190%	tp5n	nonmain_startup_fileio	windows10-64-shippable-qr	e10s stylo webrender-sw	739,361.50 -> 2,146,090.25
187%	tp5n	nonmain_startup_fileio	windows10-64-shippable	e10s stylo	736,561.25 -> 2,116,439.75
157%	tp5n	nonmain_startup_fileio	windows10-64-shippable-qr	e10s stylo webrender	737,633.92 -> 1,898,813.58
68%	tp5n	main_startup_fileio	windows10-64-shippable	e10s stylo	436,080.58 -> 730,509.25
57%	tp5n	main_startup_fileio	windows10-64-shippable-qr	e10s stylo webrender-sw	518,191.50 -> 813,901.50
55%	tp5n	main_startup_fileio	windows10-64-shippable-qr	e10s stylo webrender	518,103.67 -> 804,000.17
21%	sessionrestore_no_auto_restore	sessionrestore_no_auto_restore	linux64-shippable-qr	e10s stylo webrender-sw	483.00 -> 584.08
17%	sessionrestore_no_auto_restore	sessionrestore_no_auto_restore	linux64-shippable	e10s stylo	495.12 -> 577.75
15%	sessionrestore_no_auto_restore	sessionrestore_no_auto_restore	windows10-64-shippable	e10s stylo	508.92 -> 586.50
15%	sessionrestore_no_auto_restore	sessionrestore_no_auto_restore	windows10-64-shippable-qr	e10s stylo webrender-sw	512.83 -> 590.08
12%	sessionrestore	sessionrestore	windows10-64-shippable	e10s stylo	473.65 -> 532.17
10%	sessionrestore	sessionrestore	windows10-64-shippable-qr	e10s stylo webrender	483.24 -> 533.92
10%	sessionrestore	sessionrestore	windows10-64-shippable-qr	e10s stylo webrender-sw	494.92 -> 545.50
8%	sessionrestore	sessionrestore	linux64-shippable	e10s stylo	485.47 -> 522.42

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
32%	about_newtab_with_snippets	responsiveness	linux64-shippable-qr	e10s stylo webrender	0.25 -> 0.17
12%	startup_about_home_paint	startup_about_home_paint	linux64-shippable	e10s stylo	739.58 -> 652.25
11%	startup_about_home_paint_realworld_webextensions	startup_about_home_paint_realworld_webextensions	linux64-shippable	e10s stylo	769.33 -> 682.17
10%	startup_about_home_paint	startup_about_home_paint	windows10-64-shippable-qr	e10s stylo webrender	751.24 -> 675.92
9%	startup_about_home_paint	startup_about_home_paint	windows10-64-shippable-qr	e10s stylo webrender-sw	725.00 -> 662.00
9%	startup_about_home_paint_realworld_webextensions	startup_about_home_paint_realworld_webextensions	linux64-shippable-qr	e10s stylo webrender	948.00 -> 866.33
8%	startup_about_home_paint	startup_about_home_paint	linux64-shippable-qr	e10s stylo webrender	936.83 -> 861.25
6%	startup_about_home_paint_realworld_webextensions	startup_about_home_paint_realworld_webextensions	windows10-64-shippable	e10s stylo	758.00 -> 713.58

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7b8bd49a741
Convert SessionFile.jsm to use IOUtils r=Standard8

Comment 13

[Cosmin Sabou [:CosminS]]

Backed for causing both Bug 1735649 and Bug 1735650.

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

Failure log:

• https://treeherder.mozilla.org/logviewer?job_id=354691081&repo=autoland
• https://treeherder.mozilla.org/logviewer?job_id=354688323&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/2d35f63eeba72fe7c827c992b7b0ca8f21d57d08

Comment 14

[Mark Banner (:standard8)]

We've fixed a Marionette test issue, but we can't reproduce the main failure on try server:

https://treeherder.mozilla.org/jobs?repo=try&revision=3d33640229b5066b107d53749e343212c06c758a

We're going to try landing again and see what happens. It could have been a conflict with another patch that had landed or something like that.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0ea805e58e7
Convert SessionFile.jsm to use IOUtils r=Standard8

Comment 16

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/b0ea805e58e7

Comment 17

[Acasandrei Beatrice (needinfo me)]

Hello, Perfherder has detected these improvements, can you tell me if this patch could cause these improvements? Bug 1735500 and Bug 1736061 could be the cause as well.

Comment 18

[Marian-Vasile Laza]

I'm sorry but I can't answer this question... Aryx, could you please help us and give an answer?

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Bug 1736061 only modifies a marionette test and cannot affect performance metrics.
This bug (bug 1649599) switches the code which reads the session data to restore it. Memory usage should be higher when the page gets loaded later and even if the code used for the reading was not freed, 5% difference sounds large. A similar change (bug 1649606) has no performance alert.
Bug 1735500 changes the process type which alters what features etc. are available in the process. That one looks the most likeliest as the regressing change.

Comment 20

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Pulsebot from comment #15)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0ea805e58e7
Convert SessionFile.jsm to use IOUtils r=Standard8

== Change summary for alert #31942 (as of Mon, 18 Oct 2021 14:03:15 GMT) ==

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
45%	google-docs-canvas LastVisualChange	macosx1014-64-shippable-qr	warm webrender	1,163.33 -> 1,686.67

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
5%	wikipedia fcp	linux1804-64-shippable-qr	warm webrender	184.83 -> 174.92
4%	wikipedia fnbpaint	linux1804-64-shippable-qr	warm webrender	195.00 -> 188.00

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

Comment 21

[Andra Esanu (needinfo me)]

(In reply to Pulsebot from comment #15)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0ea805e58e7
Convert SessionFile.jsm to use IOUtils r=Standard8

== Change summary for alert #31982 (as of Wed, 20 Oct 2021 05:58:15 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
82%	sessionrestore_many_windows	windows10-64-shippable-qr	e10s stylo webrender-sw	2,323.92 -> 424.58
79%	sessionrestore_many_windows	windows10-64-shippable-qr	e10s stylo webrender	2,271.08 -> 488.25
58%	sessionrestore_many_windows	macosx1014-64-shippable-qr	e10s stylo webrender-sw	2,048.50 -> 850.92
57%	sessionrestore_many_windows	macosx1015-64-shippable-qr	e10s stylo webrender-sw	1,315.00 -> 563.83
57%	sessionrestore_many_windows	macosx1014-64-shippable-qr	e10s stylo webrender	2,261.17 -> 981.67
...	...	...	...	...
7%	sessionrestore_many_windows	macosx1015-64-shippable-qr	e10s fission stylo webrender	838.25 -> 779.83

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

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #19)

Bug 1736061 only modifies a marionette test and cannot affect performance metrics.
This bug (bug 1649599) switches the code which reads the session data to restore it. Memory usage should be higher when the page gets loaded later and even if the code used for the reading was not freed, 5% difference sounds large. A similar change (bug 1649606) has no performance alert.
Bug 1735500 changes the process type which alters what features etc. are available in the process. That one looks the most likeliest as the regressing change.

:alexandrui it looks like the investigation into this alert is incomplete, could you follow up?

Comment 23

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Uhm, seems like Beatrice updated the improvement and linked the bug to it. What do you mean more exactly?

Comment 24

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Alexandru Ionescu (needinfo me) [:alexandrui] from comment #23)

Uhm, seems like Beatrice updated the improvement and linked the bug to it. What do you mean more exactly?

Perhaps I misread the bug comments. Comment 10 suggests there were several high magnitude regressions and improvements, that were resolved by a backout shown in comment 11. When it relanded we saw a regression in comment 20 and improvements in comment 21. Was the google-canvas regression alert accepted as a wontfix?

Comment 25

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Oh, yeah, thanks for the thorough explanation. Opened a regression bug.