Implement BackgroundChild::GetOrCreateForLocalStorage

RESOLVED WONTFIX

Status

()

enhancement
RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: janv, Assigned: janv)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Originally, I wanted to fix bug 1283609, but due to time constraints, I just did local storage specific method. It's like SynchronouslyCreateForCurrentThread(), but instead of just calling GetOrCreateForCurrentThread and spinning the event loop, everything is done using thread blocking calls.

Patch coming in a sec.
(Assignee)

Updated

2 years ago
Blocks: 1350637
(Assignee)

Comment 1

2 years ago
Posted patch patchSplinter Review
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Attachment #8890429 - Flags: review?(bugmail)
Comment on attachment 8890429 [details] [diff] [review]
patch

I suspect you meant to ask :billm for review on this given the discussion on bug 1283609 and that this code seems derived from existing Background* code.  (And also because neither of us are IPC peers :)

Also, I'm not quite clear on what was broken as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1283609#c8 that merits this specialized variant.  Did you just not want to deal with the existing async consumers and their callbacks?  It seems like that's the difference between the existing dispatched-to-background-thread-then-bounced-back-to-the-main-thread RequestMessageLoopRunnable versus your new RequestMessageLoopHelper.

Also, CreateBackgroundThread is still going to create a RequestMessageLoopRunnable and dispatch it even when this new specialized-path is taken, which freaks me out.  From my perspective, it seems like it would be preferable to make us always synchronously create PBackground and use your RequestMessageLoopHelper monitor approach.  The jank delay is the time to spin up a new nsThread and dispatch a single runnable at it.  All async consumers would end up just being: `CreateSync(); NS_DispatchToMainThread(aCallback);`.  That's some jank, but appreciably less than the current RequestMessageLoopRunnable bounce.
Attachment #8890429 - Flags: review?(bugmail) → review?(wmccloskey)
(Assignee)

Comment 3

2 years ago
I just didn't want to deal with non main thread scenario, for now.

Also, if GetOrCreateForLocalStorage is called after GetOrCreateForCurrentThread, the async process already started and it would be hard to reuse the thread local stuff to look for existing background child actor.

If you guys think that there can only be one RequestMessageLoopHelper that gets the message loop by blocking the main thread in main process then I can do it that way.
Comment on attachment 8890429 [details] [diff] [review]
patch

Review of attachment 8890429 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not in favor of having this special thing just for local storage. I understand the desire to finish things quickly, but it doesn't seem like that much more work to do this the right way.

I think we could simplify this code somewhat as follows. Right now, calling Open requires a MessageLoop, and we don't have one for a thread that was just created. I would recommend adding an Open overload that takes an nsIEventTarget instead of a MessageLoop. The only use of the MessageLoop is here, as far as I can tell:
http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/ipc/glue/MessageChannel.cpp#805
We could change that to eventTarget->Dispatch instead of PostTask. For callers passing in a MessageLoop, we could call the event target version, passing messageLoop->SerialEventTarget():
http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/ipc/chromium/src/base/message_loop.h#158

As much as possible, it would be good to change the asynchronous code to use the synchronous paths. If we make the change in the previous paragraph, I can't think of any performance advantages to using the asynchronous paths anymore.
Attachment #8890429 - Flags: review?(wmccloskey) → review-
(Assignee)

Comment 5

2 years ago
Ok, I'll take a look.

However, if I understand you correctly, you want to replace:
static bool
GetOrCreateForCurrentThread(nsIIPCBackgroundChildCreateCallback* aCallback);

static PBackgroundChild*
SynchronouslyCreateForCurrentThread();

with:
static PBackgroundChild*
GetOrCreateForCurrentThread();

Having both, asynchronous and real synchronous (blocking) GetOrCreate() would be hard, since we can't reuse threadLocal->mActor for both.

This all sounds great, except it's quite a big change :)
Flags: needinfo?(wmccloskey)
Yes, although we could keep the old asynchronous API around so we don't have to change all the callers. But the asynchronous API would just invoke the synchronous API.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 7

2 years ago
Ok, one more question ...

In bug 1283609 comment 6 you gave me instructions how to fix BackgroundChild::SynchronouslyCreateForCurrentThread():
1. Create the endpoints, as we do here:
http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/ipc/glue/BackgroundImpl.cpp#1995-2004

2. Create a ChildImpl, like we do here:
http://searchfox.org/mozilla-central/source/ipc/glue/BackgroundImpl.cpp#1534

3. Bind the actor to the child endpoint, like this:
http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/ipc/glue/BackgroundImpl.cpp#1814

4. On the main thread (which might require dispatching to the main thread if we're not there already), send the parent endpoint to the parent process, like this:
http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/ipc/glue/BackgroundImpl.cpp#2006

5. Return the actor created in step 2.

Step 4 will still be asynchronous, but that's okay. It should be fine to start using the actor before step 4 has completed.


One thing is not clear to me, some other stuff must run on the main thread too, for example for calling CreateEndpoints() I need to get a process id and that is currently done by calling ContentChild::OtherPid().
Also, for the same process case, I need to call CreateBackgroundThread() which is also main thread only.
This can't be done asynchronously.

The only way how to do that is to use a monitor and block the current thread until a runnable dispatched to the main thread is executed. If we are creating a background child for the main thread, we don't need to block anything of course.
Anyway, is blocking ok for other threads, for example DOM workers ?
Flags: needinfo?(wmccloskey)
(In reply to Jan Varga [:janv] from comment #7)
> The only way how to do that is to use a monitor and block the current thread
> until a runnable dispatched to the main thread is executed. If we are
> creating a background child for the main thread, we don't need to block
> anything of course.
> Anyway, is blocking ok for other threads, for example DOM workers ?

Ugh, I don't think that's okay.

I think we should be able to call ContentChild::GetSingleton() and ContentChild::OtherPid() on a non-main thread. That data never changes after it's set, and it should be set by the time any of this happens.

CreateBackgroundThread is harder. We could make it thread-safe, but that would take some effort. What if, instead, we pass a special event target to Open that takes any runnable dispatched to it and passes it to the background thread once the background thread is ready? I think this should work, and it shouldn't be too hard to implement.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 9

2 years ago
Ok, I'll try to do it the way you suggested.
(Assignee)

Comment 10

2 years ago
(In reply to Bill McCloskey (:billm) from comment #8)
> I think we should be able to call ContentChild::GetSingleton() and
> ContentChild::OtherPid() on a non-main thread. That data never changes after
> it's set, and it should be set by the time any of this happens.

This seems to work, although I need to do more testing.

> CreateBackgroundThread is harder. We could make it thread-safe, but that
> would take some effort. What if, instead, we pass a special event target to
> Open that takes any runnable dispatched to it and passes it to the
> background thread once the background thread is ready? I think this should
> work, and it shouldn't be too hard to implement.

Now, I'm going to try this ...
(Assignee)

Comment 11

2 years ago
(In reply to Bill McCloskey (:billm) from comment #8)
> CreateBackgroundThread is harder. We could make it thread-safe, but that
> would take some effort. What if, instead, we pass a special event target to
> Open that takes any runnable dispatched to it and passes it to the
> background thread once the background thread is ready? I think this should
> work, and it shouldn't be too hard to implement.

This seems to work too :)
For now, I'm able to run local storage tests at least w/o any crashes.

I'm going to clean it up and push to try.

Bill, thanks for all the info/suggestions.
Thanks for doing this Jan! It will be great to have this done.
(Assignee)

Updated

2 years ago
No longer blocks: 1350637
(Assignee)

Comment 13

2 years ago
Bug 1283609 landed instead.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
(Assignee)

Updated

2 years ago
See Also: → 1283609
You need to log in before you can comment on or make changes to this bug.