632556 - Run nsIFile::Reveal/Launch asynchronously

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

+++ This bug was initially created as a clone of Bug #622840 +++

We shouldn't block the main thread on shell calls (which I found out today can take painfully long to complete) and both of these do.  Not sure how exactly to make these async (in other words, idk what thread the shell calls should run on) but the main thread is definitely the wrong thread!

Comment 1

[(dormant account)]

Brian can you look into this?

Comment 2

[Brian R. Bondy [:bbondy]]

This should be pretty easy to do, is this all platforms by the way or only on Windows?

Comment 3

[(dormant account)]

(In reply to Brian R. Bondy [:bbondy] from comment #2)
> This should be pretty easy to do, is this all platforms by the way or only
> on Windows?

I think we should make launch/reveal async everywhere.

rant: nsIFile is a bad place for this sort of api since pretty much every function it calls stat() -> most of these methods should not be called on the main thread. I can't imagine a crappier file api :(

Comment 4

[Brian R. Bondy [:bbondy]]

> rant: nsIFile is a bad place for this sort of api since pretty much 
> every function it calls stat() -> most of these methods should not be 
> called on the main thread. I can't imagine a crappier file api :(

I agree there is probably a better place for this call but we can move that in another bug if we want to.  Regarding every function calling stat(), this is no longer the case by the way.

> I think we should make launch/reveal async everywhere.

Since we only know for sure the slowdown is on Windows and since this code is completely different on Windows to other platforms I will just do the Windows one in the context of this task.  If we want to we can post for other platforms separately.

Comment 5

[Brian R. Bondy [:bbondy]]

By the way I profiled the Reveal call, which is called when you right click in download manager and click on open containing folder.  It takes ~210ms per call... on the main thread :(

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

Async Reveal and Launch calls.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 596516 [details] [diff] [review]
Patch v1.

Hm, this makes mAsyncOpThread in Launch() and Reveal(), and doesn't take into account the fact that mAsyncOpThread might already have been created...

Comment 8

[Brian R. Bondy [:bbondy]]

I realized that but didn't know if that would be a problem, what is the correct way to handle this?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Could we use a single global thread and create it during startup?

Comment 10

[Brian R. Bondy [:bbondy]]

If we don't already have a single global thread I wouldn't be opposed to it.  Things like jump list icon fetching could be changed to use that as well.  I suspect the code in this patch will be a common pattern as well. 

Please advise on how to proceed.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The way LazyIdleThread works is you create it once and then dispatch to it whenever you need to (just like a regular nsThread). Behind the scenes it makes an OS thread to service your runnable whenever you call Dispatch (and not before). If enough time passes with no activity the OS thread is shut down. When another runnable is posted it will recreate the OS thread, rinse and repeat.

So for now I don't see anything wrong with this:

  Reveal() {
    ...
    if (!mAsyncOpThread) {
      mAsyncOpThread = new LazyIdleThread();
    }
    mAsyncOpThread->Dispatch(...);
    ...
  }

Same for Launch.

Comment 12

[Brian R. Bondy [:bbondy]]

Comment on attachment 596516 [details] [diff] [review]
Patch v1.

k I'll change to that, thanks for the info.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Though, someday soon hopefully khuey is going to make us a File IO thread pool to handle this kind of thing ;)

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to ben turner [:bent] from comment #11)
> The way LazyIdleThread works is you create it once and then dispatch to it
> whenever you need to (just like a regular nsThread). Behind the scenes it
> makes an OS thread to service your runnable whenever you call Dispatch (and
> not before). If enough time passes with no activity the OS thread is shut
> down. When another runnable is posted it will recreate the OS thread, rinse
> and repeat.
> 
> So for now I don't see anything wrong with this:
> 
>   Reveal() {
>     ...
>     if (!mAsyncOpThread) {
>       mAsyncOpThread = new LazyIdleThread();
>     }
>     mAsyncOpThread->Dispatch(...);
>     ...
>   }
> 
> Same for Launch.

Thread safety?  At least in theory it can be called simultaneously on different threads.

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #14)
> Thread safety?  At least in theory it can be called simultaneously on
> different threads.

Um, I assumed that Reveal/Launch were all main-thread-only... Wrong? In that case I would argue that we only want to make the additional thread if we're being called on the main thread, otherwise go with the old blocking behavior. A simple NS_IsMainThread() check should be ok.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to ben turner [:bent] from comment #15)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #14)
> > Thread safety?  At least in theory it can be called simultaneously on
> > different threads.
> 
> Um, I assumed that Reveal/Launch were all main-thread-only... Wrong?

I have no idea, but nsIFiles are at least in theory threadsafe.

> In that
> case I would argue that we only want to make the additional thread if we're
> being called on the main thread, otherwise go with the old blocking
> behavior. A simple NS_IsMainThread() check should be ok.

Yeah, that sounds fine to me.

Comment 17

[Brian R. Bondy [:bbondy]]

I was about to ask if we should only do the call async if it is the main thread, but then I thought consistency of sync/async would be better no matter what thread it is being called on.  These calls though are probably always (for the time being) called on the main thread anyway.

I'll do as bent suggested since it'll avoid having to implement synchronization.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Brian R. Bondy [:bbondy] from comment #17)
> I'll do as bent suggested since it'll avoid having to implement
> synchronization.

It's a little weird to do it differently depending on which thread you're on... bsmedberg may object, in which case we could split things into sync/async variants and make the sync variant fail when called from the main thread. That's more work and would break extensions most likely.

Just make sure you document the behavior difference in the IDL, maybe bsmedberg will like it :)

Comment 19

[Brian R. Bondy [:bbondy]]

k thanks for the input. 

I'll wait for bsmedberg's feedback, I'm working on another patch at the moment anyway.  

A quick and painless way to do it would be to have the thread variable creation inside of a critical section.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Brian R. Bondy [:bbondy] from comment #19)
> A quick and painless way to do it would be to have the thread variable
> creation inside of a critical section.

No, see https://mxr.mozilla.org/mozilla-central/source/xpcom/threads/LazyIdleThread.h#61

LazyIdleThread is meant to be used from a single thread.

Comment 21

[Brian R. Bondy [:bbondy]]

Alright I vote for async call only when on main thread and document it :)
Awaiting bsmedberg

Comment 22

[Benjamin Smedberg]

I think it's fine to say that it *will* be async on the main thread and *may* block on other threads, and leave it blocking on other threads for now. Or you could audit the current in-tree callers, make sure they only occur on the main thread, and then require that the API be mainthread only.

Currently this creates the async-op-thread per-nsLocalFile. If you're going to do that, why not just create the thread per operation, and then you don't have to worry about racing to create the thread or anything like that? You can shut down the thread by posting an event back to the main thread: https://developer.mozilla.org/en/Making_Cross-Thread_Calls_Using_Runnables has sample code for that.

Comment 23

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

Implemented recommendations from Comment 22 including making it main thread only. I could not find any instances that were not main thread of either method.

Comment 24

[Benjamin Smedberg]

Comment on attachment 600092 [details] [diff] [review]
Patch v2.

In the API docs: s/on the main thread on Windows/on the main thread/. There's no reason to document that it might work on non-Windows for now.

Comment 25

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/51863ce4e87d

Comment 26

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/51863ce4e87d