nsFileChannel (called e.g. by fetch) shouldn't do main thread I/O for local files

RESOLVED FIXED in Firefox 68

Status

()

defect
P2
normal
RESOLVED FIXED
2 months ago
27 days ago

People

(Reporter: florian, Assigned: mayhemer)

Tracking

(Blocks 2 bugs, {regression})

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox66 wontfix, firefox67 wontfix, firefox68 fixed)

Details

(Whiteboard: [fxperf:p2][qf:p3:responsiveness][necko-triaged])

Attachments

(1 attachment)

(Reporter)

Description

2 months ago

Near the end of startup (I think when loading activity stream), there are 21 stat calls for png files in the thumbnails folder of the profile. Apparently we do 3 stat calls per local file we are fetching.

static bool SandboxFetchPromise(struct JSContext *, unsigned int, union JS::Value *) [xul.dll]
mozilla::dom::FetchRequest(nsIGlobalObject *,mozilla::dom::RequestOrUSVString const &,mozilla::dom::RequestInit const &,mozilla::dom::CallerType,mozilla::ErrorResult &) [xul.dll]
nsresult mozilla::dom::FetchDriver::Fetch(class mozilla::dom::AbortSignalImpl *, class mozilla::dom::FetchDriverObserver *) [xul.dll]
nsresult mozilla::dom::FetchDriver::HttpFetch(const class nsTSubstring<char> & const) [xul.dll]
nsBaseChannel::AsyncOpen(nsIStreamListener *) [xul.dll]
nsresult nsFileChannel::OpenContentStream(bool, class nsIInputStream * *, class nsIChannel * *) [xul.dll]
nsresult nsLocalFile::GetFileSize(__int64 *) [xul.dll]
nsLocalFile::ResolveAndStat []
nsresult nsLocalFile::ResolveAndStat() [xul.dll]
static nsresult GetFileInfo(const class nsTString<char16_t> & const, struct PRFileInfo64 *) [xul.dll]
GetFileAttributesExW [KERNELBASE.dll]
nsThread::ProcessNextEvent(bool,bool *) [xul.dll]
nsresult mozilla::dom::`anonymous namespace'::BeginConsumeBodyRunnable<mozilla::dom::Response>::Run() [xul.dll]
nsresult mozilla::dom::FetchBodyConsumer<mozilla::dom::EmptyBody>::GetBodyLocalFile(class nsIFile * *) [xul.dll]
nsLocalFile::Exists(bool *) [xul.dll]
nsLocalFile::ResolveAndStat []
nsresult nsLocalFile::ResolveAndStat() [xul.dll]
static nsresult GetFileInfo(const class nsTString<char16_t> & const, struct PRFileInfo64 *) [xul.dll]
GetFileAttributesExW [KERNELBASE.dll]

This is at https://searchfox.org/mozilla-central/rev/01b4b3830ea3cae2e9e431019afa6391b471c6da/dom/fetch/FetchConsumer.cpp#505 and looks like a regression from bug 1482752.

nsThread::ProcessNextEvent(bool,bool *) [xul.dll]
nsresult mozilla::dom::`anonymous namespace'::BeginConsumeBodyRunnable<mozilla::dom::Response>::Run() [xul.dll]
mozilla::dom::FileCreatorHelper::CreateFile(nsIGlobalObject *,nsIFile *,mozilla::dom::ChromeFilePropertyBag const &,bool,mozilla::ErrorResult &) [xul.dll]
static struct already_AddRefed<mozilla::dom::File> mozilla::dom::FileCreatorHelper::CreateFileInternal(class nsPIDOMWindowInner *, class nsIFile *, const struct mozilla::dom::ChromeFilePropertyBag & const, bool, class mozilla::ErrorResult & const) [xul.dll]
static nsresult mozilla::dom::FileCreatorHelper::CreateBlobImpl(class nsIFile *, const class nsTSubstring<char16_t> & const, const class nsTSubstring<char16_t> & const, bool, __int64, bool, bool, class mozilla::dom::BlobImpl * *) [xul.dll]
nsresult mozilla::dom::MultipartBlobImpl::InitializeChromeFile(class nsIFile *, const class nsTSubstring<char16_t> & const, const class nsTSubstring<char16_t> & const, bool, __int64, bool) [xul.dll]
nsLocalFile::Exists(bool *) [xul.dll]
nsLocalFile::ResolveAndStat []
nsresult nsLocalFile::ResolveAndStat() [xul.dll]
static nsresult GetFileInfo(const class nsTString<char16_t> & const, struct PRFileInfo64 *) [xul.dll]
GetFileAttributesExW [KERNELBASE.dll]

This is at https://searchfox.org/mozilla-central/rev/01b4b3830ea3cae2e9e431019afa6391b471c6da/dom/file/MultipartBlobImpl.cpp#331

All this information is from this profile: https://perfht.ml/2DQYNoG

(Assignee)

Comment 1

2 months ago

Good findings. This is known, I think, and not easy to act on, I'm afraid. Moving to Net:File as there would probably be the fix.

Tentatively P1, as I'd like to at least start discussion on how to improve the situation here.

Component: DOM: Networking → Networking: File
Priority: -- → P1
Summary: fetch shouldn't do main thread I/O for local files → nsFileChannel (called e.g. by fetch) shouldn't do main thread I/O for local files
Whiteboard: [fxperf][qf] → [fxperf][qf][necko-triaged]
(Assignee)

Updated

2 months ago
Assignee: nobody → honzab.moz

Could we make nsBaseChannel::BeginPumpingData() be called asynchronously on another thread? And if it fails we fail the channel?

Updated

2 months ago
Whiteboard: [fxperf][qf][necko-triaged] → [fxperf][qf:p3:responsiveness][necko-triaged]
Whiteboard: [fxperf][qf:p3:responsiveness][necko-triaged] → [fxperf:p2][qf:p3:responsiveness][necko-triaged]

Updated

a month ago
Blocks: 1534603

Comment 3

a month ago

Note that the top stack here:

nsBaseChannel::AsyncOpen(nsIStreamListener *) [xul.dll]
nsresult nsFileChannel::OpenContentStream(bool, class nsIInputStream * *, class nsIChannel * *) [xul.dll]
nsresult nsLocalFile::GetFileSize(__int64 *) [xul.dll]
nsLocalFile::ResolveAndStat []
nsresult nsLocalFile::ResolveAndStat() [xul.dll]
static nsresult GetFileInfo(const class nsTString<char16_t> & const, struct PRFileInfo64 *) [xul.dll]
GetFileAttributesExW [KERNELBASE.dll]

affects XHRs, too.

(Assignee)

Updated

a month ago
Blocks: 1534712
(Assignee)

Comment 4

a month ago

We definitely also stat the file from nsFileChannel::Init, I'm surprised you didn't catch that as well. I think we need the symlink resolved uri prior calling asyncopen to be able to do proper security checking. anyway, this is hard and sensitive to fix while we create channel and do secchecks on the main thread, in one loop. as it's not listed in the stacks in comment 0 or later on, I will dedicate this bug to an attempt to get rid of at least those stats we do from inside asyncOpen. That should be doable relatively easily.

(Assignee)

Comment 6

a month ago

Seems to cause a permafailure:
https://bugzilla.mozilla.org/show_bug.cgi?id=1471737, browser/components/preferences/in-content/tests/browser_subdialogs.js

(Assignee)

Comment 7

a month ago

I believe the defect is in the browser_subdialogs.js test itself. I double checked that all channel finish loading and also tested to do the background IO loop blocking call of asyncOpen while also doing a sync resolution of mContentLength as before just in case someone wants the content length be known before OnStartRequest call.

The test is apparently time sensitive.

Depends on: 1471737

Comment 8

a month ago

Please let me know if I can help in some way to get that test to behave better.

(Assignee)

Comment 9

a month ago

(In reply to :Gijs (he/him) from comment #8)

Please let me know if I can help in some way to get that test to behave better.

I already ni?'ed few people that have changed the test recently, maybe I'll get some answer soon. I also raised the priority to reflect this bug.

I think I should do some talos pushes too... also, stream transport service pool can create (and probably immediately destroy) ~400 threads during that single local test run; something I'm going to look into now as it doesn't seem OK to me at all.

(Assignee)

Comment 10

a month ago

(In reply to :Gijs (he/him) from comment #8)

Please let me know if I can help in some way to get that test to behave better.

If you want, just grab bug 1471737. I added few comments with some initial pointers.

(Assignee)

Updated

a month ago
Depends on: 1535361
(Assignee)

Comment 12

a month ago

The talos compare shows some weird regression only on win7-32, which is probably not something caused by this patch.

Let's land it.

Keywords: checkin-needed

Comment 13

a month ago

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2657a29a6d76
Move I/O in nsFileChannel::OpenContentStream to a background thread, r=kershaw,dragana

Keywords: checkin-needed

Comment 14

a month ago
bugherder
Status: NEW → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

67=wontfix because there's no need to uplift to 67 Beta

(Assignee)

Comment 16

29 days ago

Backed out in bug 1538535 for possible cause of bug 1538098.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 17

28 days ago

We can reland, as this has been proven to NOT cause bug 1538098 (https://bugzilla.mozilla.org/show_bug.cgi?id=1538535#c3).

I'd rather wait a bit until m-c stabilize again.

(Assignee)

Comment 18

28 days ago

Problem identified elsewhere.

Comment 19

27 days ago
Pushed by honzab.moz@firemni.cz:
https://hg.mozilla.org/integration/autoland/rev/d121ec2b3d8a
Move I/O in nsFileChannel::OpenContentStream to a background thread, r=kershaw,dragana

Comment 20

27 days ago
bugherder
Status: REOPENED → RESOLVED
Last Resolved: a month ago27 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.