While investigating bug 1110624 I found parent process reads application archive for the splash icon (by system app), but the opened fd is not reused by child process. Neither OpenFileAndSendFDRunnable nor RemoteOpenFileParent::OpenSendCloseDelete() checks the JAR cache. Though currently JAR cache does not keep the fd by default, but it will once bug 1069081 is landed. Then the fd can be a parameter in SendLoadURL for child process, and we can eliminate extra file opening and possibly dealy from IPC.

(In reply to Ting-Yu Chou [:ting] from comment #0) > While investigating bug 1110624 I found parent process reads application > archive for the splash icon (by system app), but the opened fd is not reused > by child process. Neither OpenFileAndSendFDRunnable nor > RemoteOpenFileParent::OpenSendCloseDelete() checks the JAR cache. I just learnt from Thinker that even the fd is cloned for child process, the file pointer is shared, so there will be problems if both content/child are accessing the file. That means the fd from JAR cache can not be used. I'll double check this. Thinker has another idea is to let RemoteOpenFileParent::OpenSendCloseDelete() to be done on other than B2G main thread, so it won't be blocked by other processings.

(In reply to Ting-Yu Chou [:ting] from comment #1) > file pointer is shared, so there will be problems if both content/child are s/content/parent/

(In reply to Ting-Yu Chou [:ting] from comment #1) > I just learnt from Thinker that even the fd is cloned for child process, the > file pointer is shared, so there will be problems if both content/child are > accessing the file. That means the fd from JAR cache can not be used. I'll > double check this. Yes, the offset will be shared. But parent does not need to access the file after reading the icon, it can clone to child and close.

OpenFileAndSendFDRunnable 1) dispatch to StreamTrans thread pool to open the file, 2) dispatch back to main thread to send the fd, and 3) dispatch to StreamTrans service again to close it.

(In reply to Ting-Yu Chou [:ting] from comment #4) > OpenFileAndSendFDRunnable 1) dispatch to StreamTrans thread pool to open the > file, 2) dispatch back to main thread to send the fd, and 3) dispatch to > StreamTrans service again to close it. In contrary, RemoteOpenFileParent::OpenSendCloseDelete() opens the file on B2G main thread.

(In reply to Ting-Yu Chou [:ting] from comment #5) > (In reply to Ting-Yu Chou [:ting] from comment #4) > > OpenFileAndSendFDRunnable 1) dispatch to StreamTrans thread pool to open the > > file, 2) dispatch back to main thread to send the fd, and 3) dispatch to > > StreamTrans service again to close it. > > In contrary, RemoteOpenFileParent::OpenSendCloseDelete() opens the file on > B2G main thread. Bent, do you rememeber why does OpenFileAndSendFDRunnable open the file in StreamTrasn thread pool instead of in main thread? Thanks.

IIRC opening the file requires a stat(), so we try to avoid doing that on the main thread.

Thanks Kyle! I just checked OpenFileAndSendFDRunnable::OpenFileImpl(), NS_NewLocalFile(), and nsLocalFile::OpenNSPRFileDesc() but didn't see a stat().

It's not a stat we're worried about. Windows calls CreateFile, which can block, and POSIX calls open/open64, which can block.

Also, OpenSendCloseDelete just needs to be fixed to not do main thread I/O. See http://mxr.mozilla.org/mozilla-central/source/netwerk/ipc/RemoteOpenFileParent.cpp#33

I have tried to measure PR_Open() (which uses open()) on Flame, it takes less than 1ms, usually ~0.6ms.

(In reply to Ting-Yu Chou [:ting] from comment #11) > I have tried to measure PR_Open() (which uses open()) on Flame, it takes > less than 1ms, usually ~0.6ms. I couldn't find a statement that CreateFile/open/open64 an existing file would be blocked. Bent, do you know when could that happen?

I am not really sure, but I guess :bent means CreateFile/open/open64 are synchronous and can block main thread, even it is not long.

On my Flame, the fd arrives >30ms after RecvLoadURL() and sometimes can even >90ms.

Bug 984050 just added a BrowserConfiguration paramter to LoadURL().

Per comment 11, 12, and 14, is opening an existed file on main thread really a concern?

(In reply to Ting-Yu Chou [:ting] from comment #16) > Per comment 11, 12, and 14, is opening an existed file on main thread really > a concern? Yes. We don't allow new main-thread I/O of any kind unless there's a seriously good reason for it.

I'll see if we can share the mmapped JAR to content process instead.

When the app has isCoreApp true [1] (stores under /system/b2g/webapps), it will have uri "jar://..." not "jar:remoteopenfile://" which reading the archive does not depend on receiving JAR file fd (TabChild::RecvCacheFileDescriptor or RemoteOpenFileChild::Recv__delete__). [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/app/AppProtocolHandler.cpp#448

(In reply to Ting-Yu Chou [:ting] from comment #19) > When the app has isCoreApp true [1] (stores under /system/b2g/webapps), it > will have uri "jar://..." not "jar:remoteopenfile://" which reading the > archive does not depend on receiving JAR file fd > (TabChild::RecvCacheFileDescriptor or RemoteOpenFileChild::Recv__delete__). > > [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/app/AppProtocolHandler.cpp#448 Does anything break if that's changed to use remoteopenfile: for all out-of-process apps? It would be nice if I didn't have to make the sandboxing code care about a special case for “core apps” — especially because “core apps” don't exist on eng builds (see also bug 1160322 comment #11).

NeckoParent::AllocPRemoteOpenFileParent looks as if it should correctly handle opening files in the “core apps” directory, but I haven't tested it yet.

(In reply to Jed Davis [:jld] {UTC-7} from comment #20) > (In reply to Ting-Yu Chou [:ting] from comment #19) > > When the app has isCoreApp true [1] (stores under /system/b2g/webapps), it > > will have uri "jar://..." not "jar:remoteopenfile://" which reading the > > archive does not depend on receiving JAR file fd > > (TabChild::RecvCacheFileDescriptor or RemoteOpenFileChild::Recv__delete__). > > > > [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/app/AppProtocolHandler.cpp#448 > > Does anything break if that's changed to use remoteopenfile: for all > out-of-process apps? It would be nice if I didn't have to make the > sandboxing code care about a special case for “core apps” — especially > because “core apps” don't exist on eng builds (see also bug 1160322 comment > #11). Fabrice, how do you think?

The core apps are placed in different directories on different devices (eng build)... Nexus5: /system/b2g/webapps Flame: /data/local/webapps

Why do we need to open applicatin archive in chrome process? If we can open core apps in content process, why not the others? I don't see any security check in OpenFileAndSendFDRunnable.

(In reply to Ting-Yu Chou [:ting] from comment #22) > (In reply to Jed Davis [:jld] {UTC-7} from comment #20) > > (In reply to Ting-Yu Chou [:ting] from comment #19) > > > When the app has isCoreApp true [1] (stores under /system/b2g/webapps), it > > > will have uri "jar://..." not "jar:remoteopenfile://" which reading the > > > archive does not depend on receiving JAR file fd > > > (TabChild::RecvCacheFileDescriptor or RemoteOpenFileChild::Recv__delete__). > > > > > > [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/app/AppProtocolHandler.cpp#448 > > > > Does anything break if that's changed to use remoteopenfile: for all > > out-of-process apps? It would be nice if I didn't have to make the > > sandboxing code care about a special case for “core apps” — especially > > because “core apps” don't exist on eng builds (see also bug 1160322 comment > > #11). > > Fabrice, how do you think? Yeah, that should work, but really we should not have eng builds putting core apps in /data anymore. (In reply to Ting-Yu Chou [:ting] from comment #24) > Why do we need to open applicatin archive in chrome process? If we can open > core apps in content process, why not the others? I don't see any security > check in OpenFileAndSendFDRunnable. That's because app processes can't read /data/local where the zips are. Only the parent can, so we open the fd in the parent and send it to the child. On the other hand, everyone can read core apps since they are on the /system partition which is read-only.

(In reply to Ting-Yu Chou [:ting] from comment #23) > The core apps are placed in different directories on different devices (eng > build)... > > Nexus5: /system/b2g/webapps > Flame: /data/local/webapps This is incorrect. After a clean flash, the apps are located under /data/local/webapps, and after |make raptor| they'll move to /system/b2g/webapps (on both devices).

Try to get fd from jar cache and send it. This works with attachment 8640424 [details] [diff] [review] (bug 1069081 which always keeps fd in jar cache).

(In reply to Ting-Yu Chou [:ting] from comment #27) > Created attachment 8640947 [details] [diff] [review] > wip > > Try to get fd from jar cache and send it. This works with attachment 8640424 [details] [diff] [review] > [details] [diff] [review] (bug 1069081 which always keeps fd in jar cache). The remaining work is to avoid async fd notification if we can check received file descriptor with TabChild earlier.

(In reply to Ting-Yu Chou [:ting] from comment #28) > The remaining work is to avoid async fd notification if we can check > received file descriptor with TabChild earlier. I tried but this doesn't make any differences.

Assigning mListener in RemoteOpenFileChild::AsyncRemoteFileOpen() earlier is a bugfix, since the listener is required if TabChild have received the fd already.

Ting-Yu, are these changes improving performance? Or are we doing them only for the reason cited in comment 20?

Part1 (attachment 8648567 [details] [diff] [review]) can earn us ~10ms on aries when launch non-core apps (remotejar). And yes, part2 (attachment 8648572 [details] [diff] [review]) is to address comment 20, but also because of part1, launch core apps will still be as fast as before.

Comment on attachment 8648572 [details] [diff] [review] part2: always use jar:remoteopenfile: for out-of-process apps Review of attachment 8648572 [details] [diff] [review]: ----------------------------------------------------------------- ok then!

Comment on attachment 8648567 [details] [diff] [review] part1: get cached jar fd instead of always openning it PRFileDesc *cachedFd = nullptr; -> PRFileDesc* cachedFd = nullptr; I don't understand the change to netwerk/. So, asking review from jduell.

Comment on attachment 8648567 [details] [diff] [review] part1: get cached jar fd instead of always openning it Review of attachment 8648567 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. As you've already mentioned, reused fd's share a single file pointer, so if something breaks from landing this as a result, that's a likely suspect (and could be ground for rolling back this change).

Addressed comment 35, updated reviewers.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8010964bd288

Try looks good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d75db15b6ed https://hg.mozilla.org/integration/mozilla-inbound/rev/1620bf9f8f2a

Backed out for nsZipReaderCache::GetFd crashes. https://treeherder.mozilla.org/logviewer.html#?job_id=13327204&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc523df399d

GetFd() does not work on Windows, wrap the code by define. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fc4c042918a

(In reply to Ting-Yu Chou [:ting] from comment #42) > Created attachment 8653262 [details] [diff] [review] > part1 v3: get cached jar fd instead of always openning it > > GetFd() does not work on Windows, wrap the code by define. > > Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fc4c042918a Looks good, tested windows and osx. Thank you, Sheriff!

https://hg.mozilla.org/integration/mozilla-inbound/rev/64383ee12257 https://hg.mozilla.org/integration/mozilla-inbound/rev/905ad90eaf00

https://hg.mozilla.org/mozilla-central/rev/64383ee12257 https://hg.mozilla.org/mozilla-central/rev/905ad90eaf00

:jld, just want to let you know your suggestion in comment 20 has been done.