Open Bug 1661941 Opened 4 years ago Updated 2 years ago

WebBrowserPersistSerialize actor does mainthread IO

Categories

(Core :: DOM: Core & HTML, defect)

defect

Tracking

()

Performance Impact low
Tracking Status
firefox82 --- affected

People

(Reporter: Gijs, Unassigned)

References

Details

(Keywords: main-thread-io, perf:responsiveness)

The serialization actor for remote documents, implemented at https://searchfox.org/mozilla-central/source/dom/webbrowserpersist/WebBrowserPersistSerializeParent.cpp , writes directly to a file stream from the actor, whose messages are received on the main thread.

I'm torn on how to fix this. The surrounding process is already asynchronous as the data to write comes from the child, and each instance notifies its creator (the caller of WriteContent on WebBrowserPersistRemoteDocument) when it's done. The two obvious routes I see to take the file writing off the main thread are:

  1. move the serialization actor to be managed by PBackground so that the messages from the child come in away from the main thread, and just write from there. Then post a runnable back to the main thread when the actor is finished.
  2. keep the actor how it is but just have it queue the actual writing tasks by moving the buffers onto the a taskqueue (so they execute serially) on the background IO thread.

The first seems the more idiomatic, but I don't actually know how the IPC background stuff works, and if doing IO on that thread is going to have other bad effects (seems we also use it for vsync? That seems scary...). The relevant code that currently instantiates the serializer actor also does so directly, and the alloc helper at https://searchfox.org/mozilla-central/rev/969fc7fa6c3c7fc489f53b7b7f8c902028b5169f/dom/webbrowserpersist/WebBrowserPersistDocumentParent.cpp#86-89 MOZ_CRASHes. I don't know why this arrangement was chosen and if it means moving to PBackground is going to cause other issues - looks like :jld implemented this, so I'm hoping you know!

The second is less idiomatic / more fussy, but otherwise straightforward - I'm adding a queue on the webbrowserpersist instance in bug 1658202 anyway, so AFAICT it'd just be a question of std::move ing the data to a runnable that gets dispatched to the other thread and writes there, and doing the same when finishing - though we may have to dispatch twice, and the actual data will first come in on the main thread, so if we want to avoid that and have a little neater code, perhaps the first option is better.

:jld, :mattwoodrow, can you point the way? :-)

Flags: needinfo?(matt.woodrow)
Flags: needinfo?(jld)

Somewhat confusingly (I had to look it up), the thread for PBackground ("IPDL Background"), is different from the Background IO thread ("IOUtils::BackgroundIOThread", a task queue running on the global "BackgroundThreadPool" thread pool).

Using PBackground is generally the idiomatic solution for "I don't want my messages to be delayed by a potentially contended main thread", and is indeed used for things like VSync messages.

Since your case is more "I don't want to be the one consuming the main thread", then I'd say moveing ownership into a runnable that gets sent to the Background IO task queue will be sufficient and easier to write.

Flags: needinfo?(matt.woodrow)

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)

Somewhat confusingly (I had to look it up), the thread for PBackground ("IPDL Background"), is different from the Background IO thread ("IOUtils::BackgroundIOThread", a task queue running on the global "BackgroundThreadPool" thread pool).

Something something we have too many threads. Because I was imprecise about the "background IO" thread I meant - I think we've generally been trying to use NS_CreateBackgroundTaskQueue and NS_DispatchBackgroundTask for this, which are implemented through nsThreadUtils, see doc comments at https://searchfox.org/mozilla-central/rev/969fc7fa6c3c7fc489f53b7b7f8c902028b5169f/xpcom/threads/nsThreadUtils.h#1756-1783 . I'm... actually not 100% sure what thread names those get.

Using PBackground is generally the idiomatic solution for "I don't want my messages to be delayed by a potentially contended main thread", and is indeed used for things like VSync messages.

Since your case is more "I don't want to be the one consuming the main thread", then I'd say moveing ownership into a runnable that gets sent to the Background IO task queue will be sufficient and easier to write.

OK, thanks!

Flags: needinfo?(jld)
Severity: -- → S3

looks like :jld implemented this

The original WebBrowserPersist was implemented in the early 2000s (bug 46574, bug 77909, etc.), and my understanding is that it did approximately everything on the main thread back then. Once it more or less worked by the standards of Web content at the time, it was mostly untouched until I refactored it for e10s in 2015 (bug 1101100), and I tried not to change things that didn't need to be changed for that.

But that was then and this is now. In addition to what's already been mentioned, another reason to avoid blocking the IPDL Background thread: it's shared among all PBackground instances in all processes (in addition to PBackground being shared by a lot of unrelated subprotocols that just need to run on some maybe-not-main thread, which is also a problem), so that's likely to cause problems.

(It would be nice to break out all of those protocols and move them all to separate task queues in a thread pool or something, but that would be a significant redesign, and I'm told there are subtle ordering / single-threadedness dependencies in some of them.)

Whiteboard: [qf] → [qf:p3:responsiveness]
Performance Impact: --- → P3
Whiteboard: [qf:p3:responsiveness]
You need to log in before you can comment on or make changes to this bug.