Closed Bug 1649599 Opened 2 years ago Closed 1 year ago

SessionFile.jsm imports OS.File during startup

Categories

(Toolkit :: OS.File, task)

task

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox95 --- fixed

People

(Reporter: barret, Assigned: masterwayz)

References

Details

(Keywords: perf-alert)

Attachments

(1 file)

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]
Depends on: 1660835
Depends on: 1671035
Depends on: 1674311
Assignee: nobody → michael
Status: NEW → ASSIGNED
Pushed by emalysz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17448bde9d5d
Convert SessionFile.jsm to use IOUtils r=emalysz

I've been looking into this with emalysz.

Need to do more investigative work on this one.

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.

Flags: needinfo?(michael)
Pushed by emalysz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/453811f08638
Convert SessionFile.jsm to use IOUtils r=emalysz
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch

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

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 86 Branch → ---

(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

(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

Keywords: perf-alert
Regressions: 1684671
Attachment #9192794 - Attachment description: Bug 1649599 - Convert SessionFile.jsm to use IOUtils r=emalysz → WIP: Bug 1649599 - Convert SessionFile.jsm to use IOUtils r=Standard8
Depends on: 1735500
Attachment #9192794 - Attachment description: WIP: Bug 1649599 - Convert SessionFile.jsm to use IOUtils r=Standard8 → Bug 1649599 - Convert SessionFile.jsm to use IOUtils r=Standard8
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c7b8bd49a741
Convert SessionFile.jsm to use IOUtils r=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.

Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0ea805e58e7
Convert SessionFile.jsm to use IOUtils r=Standard8
Status: REOPENED → RESOLVED
Closed: 2 years ago1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch

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.

Flags: needinfo?(mlaza)

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

Flags: needinfo?(mlaza) → needinfo?(aryx.bugmail)

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.

Flags: needinfo?(aryx.bugmail)

(In reply to Pulsebot from comment #15)

Pushed by mbanner@mozilla.com:
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

(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?

Flags: needinfo?(aionescu)

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

Flags: needinfo?(aionescu) → needinfo?(dave.hunt)

(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?

Flags: needinfo?(dave.hunt) → needinfo?(aionescu)

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

Flags: needinfo?(aionescu)
You need to log in before you can comment on or make changes to this bug.