BrowserThread on iOS

[François Doray]

I was recently CCed on a web::WebThread bug https://crbug.com/926785. I realized that we keep investing time in web::WebThread, which is just a copy of content::BrowserThread:

• gab 
• fdoray  
  
• altimin 
• eseckler  
  

So I decided to make a proof of concept CL that moves content::BrowserThread to //components and uses it on iOS instead of web::WebThread.

The BrowserUIThreadScheduler could probably be moved to that component so that we have the same API and implementation for the UI/IO threads on all platforms.

What do you think of this change?

[Sami Kyostila]

This seems like a good thing -- assuming it doesn't tie our hands too much with what we can do in the UI thread scheduler? We also recently came to the conclusion that we probably want a scheduler on the IO thread too. I wonder if this helps with that?

- Sami

--
You received this message because you are subscribed to the Google Groups "scheduler-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scheduler-de...@chromium.org.
To post to this group, send email to schedu...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CAGD3t5GiMaajMuQ5Y7yo8hA%3DeQsi0Z%3DMNK8wX3Ht%2BYiaLtObHg%40mail.gmail.com.

[Chris Hamilton]

Slightly O/T:

The IO scheduler was originally nixed because most of the issues appeared to be due to the network stack, which is moving out of process (very soon now!). Is there new evidence to show that the IO thread is an issue?

(FWIW, I believe it will be an issue, and that we'll want a scheduler. We've found some gnarly cases of the IO thread blocking multiple other threads in some recent trace deep dives.)

Cheers,

Chris

[Sami Kyostila]

pe 15. helmik. 2019 klo 16.36 Chris Hamilton (chr...@google.com) kirjoitti:

Slightly O/T:

The IO scheduler was originally nixed because most of the issues appeared to be due to the network stack, which is moving out of process (very soon now!). Is there new evidence to show that the IO thread is an issue?

(FWIW, I believe it will be an issue, and that we'll want a scheduler. We've found some gnarly cases of the IO thread blocking multiple other threads in some recent trace deep dives.)

We recently looked at startup traces on Android and found that there is a lot of traffic on the various IO threads during early initialization. A lot of it is networking which I hope the network service will alleviate, but there's also a lot of general mojo-related chatter. We'll come back with some more data about this.

- Sami

[Alexander Timin]

Here is the critical path for the resulting trace (already with some IO thread scheduling policy): https://pastebin.com/i8y29EbR

In particular, we can shave off extra ~65ms by having a better scheduling policy on the IO thread, for example:

- 16ms is lost due to not prioritising net/extras/sqlite/sqlite_persistent_cookie_store.cc:LoadKeyAndNotifyInBackground

- 13ms from net/http/http_cache.cc:OnBackendCreated

- 10ms from net/disk_cache/simple/simple_backend_impl.cc:Init

- 7ms from content/browser/loader/navigation_url_loader_impl.cc:FollowRedirect

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CAPuLczuJFQ%3Dx5KWe_dsfRauuFGNcixfJSOJepwDGz1wQEd8PgQ%40mail.gmail.com.

[Chris Hamilton]

The first 3 are all in net and are moving out of process. Likely we'll still want to schedule those in the net process, but at least they'll be out of the browser startup critical path.

Which brings up the question: How much work will it be to make sequence manager work for main threads out of the browser process? We'll likely want to be able to have per-process type main thread schedulers.

[Sami Kyostila]

pe 15. helmik. 2019 klo 17.00 Chris Hamilton (chr...@google.com) kirjoitti:

The first 3 are all in net and are moving out of process. Likely we'll still want to schedule those in the net process, but at least they'll be out of the browser startup critical path.

Which brings up the question: How much work will it be to make sequence manager work for main threads out of the browser process? We'll likely want to be able to have per-process type main thread schedulers.

I'm glad you asked :) As a part of the UI thread scheduler work we've been cleaning up and unifying the underlying task scheduling primitives in //base, and in fact now all (non-worker pool) threads have a SequenceManager on them. This makes building schedulers for specific processes or threads much easier.

- Sami 

[Sami Kyostila]

pe 15. helmik. 2019 klo 17.16 Sami Kyostila (skyo...@chromium.org) kirjoitti:

pe 15. helmik. 2019 klo 17.00 Chris Hamilton (chr...@google.com) kirjoitti:

The first 3 are all in net and are moving out of process. Likely we'll still want to schedule those in the net process, but at least they'll be out of the browser startup critical path.

Sorry, forgot to mention that we're measuring startup as time from launch to first contentul paint on whatever page you're loading up, so the net stack is still on the critical path even if it moves out of process.

[Alexander Timin]

And most of the delay for the net tasks comes from other net tasks, so even when it moves out of process (which AFAIK isn't going to happen for low-end Android), we still have the same problem.

[Alex Clarke]

On Fri, 15 Feb 2019 at 17:00, 'Chris Hamilton' via scheduler-dev <schedu...@chromium.org> wrote:

The first 3 are all in net and are moving out of process. Likely we'll still want to schedule those in the net process, but at least they'll be out of the browser startup critical path.

I was under the impression the network service was going to be in process on android due to memory overhead. (I could be wrong here!)

Adding an IO scheduler is simple now.  E,g. https://chromium-review.googlesource.com/c/chromium/src/+/1463560

 

Which brings up the question: How much work will it be to make sequence manager work for main threads out of the browser process? We'll likely want to be able to have per-process type main thread schedulers.

Should be straight forward.  I wonder if it will be useful in the net process (seems like it might), what about the GPU process?

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CAA2pCLdD5qN0HLOu_UbzCCtcM%3DZQcaEOs6yn-DKb57tLHeioOg%40mail.gmail.com.

[Eugene But]

Hi François,

It would be really amazing if we could move threading code out of //ios/web. Web Layer should be responsible for managing web contents and navigation, and threading support is out of scope for this layer. There are a few constraints however:

  - //content and //ios/web can not use //components (existing examples are layering violations)

  - //ios/web can not use //content (which is currently true)

//content and //ios/web can use the code from //base. So maybe we could move all shared code to //base?

Adding thankis@ for evaluating if //base is the right place for threading code. blundell@ who designed layered components. rohitrao@ who is probably the only engineer on iOS team who understands //ios/web threading code.

Thanks,

Eugene

[Chris Hamilton]

On Fri, Feb 15, 2019 at 12:22 PM Alex Clarke <alexc...@google.com> wrote:

On Fri, 15 Feb 2019 at 17:00, 'Chris Hamilton' via scheduler-dev <schedu...@chromium.org> wrote:

The first 3 are all in net and are moving out of process. Likely we'll still want to schedule those in the net process, but at least they'll be out of the browser startup critical path.

I was under the impression the network service was going to be in process on android due to memory overhead. (I could be wrong here!)

This is an important fact. The original resistance by jam@ to having an IO scheduler (way back when gab@ did eng review on this work) was because net was moving OOP.

 

Adding an IO scheduler is simple now.  E,g. https://chromium-review.googlesource.com/c/chromium/src/+/1463560

 

Which brings up the question: How much work will it be to make sequence manager work for main threads out of the browser process? We'll likely want to be able to have per-process type main thread schedulers.

Should be straight forward.  I wonder if it will be useful in the net process (seems like it might), what about the GPU process?

+Etienne Bergeron for more info here. He's been working with folks on GPU team in trying to improve things where the cause of main thread jank in the browser is because of work being done in the GPU process. He might have something thoughts here. My guess is that almost certainly main and IO thread scheduling will be useful in all non-trivial process types.

[Chris Hamilton]

Good point. Which means we'll need a main thread net service scheduler :)

FWIW, we've been looking at the cookie problem and we think there's lots of room for improvement here. At early startup we know exactly which URLs are going to load first and can prioritize those origins. Similarly, we have all sorts of data as to popularity of any given origin in any given web request, and could sort the entire cookie store database based on popularity. Finally, we could make the cookie store loading asynchronous to actual web requests being issued, and effectively wait on the origin in question having been loaded (or end of loading if that origin has never been seen before) prior to making any net request. The common case will be that the cookie store can continue lazily loading as a best effort task after a few high priority origins have been read.

Cheers,

Chris

[Gabriel Charette]

On Fri, Feb 15, 2019 at 12:30 PM Eugene But <euge...@chromium.org> wrote:

Hi François,

It would be really amazing if we could move threading code out of //ios/web. Web Layer should be responsible for managing web contents and navigation, and threading support is out of scope for this layer. There are a few constraints however:

  - //content and //ios/web can not use //components (existing examples are layering violations)

  - //ios/web can not use //content (which is currently true)

//content and //ios/web can use the code from //base. So maybe we could move all shared code to //base?

Adding thankis@ for evaluating if //base is the right place for threading code. blundell@ who designed layered components. rohitrao@ who is probably the only engineer on iOS team who understands //ios/web threading code.

No //base isn't the right place for BrowserThreads. Why can't we have a basic component for this? I thought this was the whole point of components but I keep being confused at why components can't be used in various places. Feels against base's principles of not being a dumpground to ban extracting a common component from //content and //ios/web (which are currently out-of-phase copies of each other..).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CACiDxEFGFajMKgeTJpH-wF8xL577qQ65Djp%3D2J0jR0VGfitsQQ%40mail.gmail.com.

[Gabriel Charette]

+Rohit Rao who was working on making //ios/web catchup with //content threading in 2018 (but I guess we didn't finish that if discrepancy is still a problem?)

[Eugene But]

By design //components layer sits above //content and //ios/web. Higher layer can use the code from lower layer:

chrome

components

content

net

base

[Nico Weber]

I thought that used to be true long ago but isn't any more: https://cs.chromium.org/search/?q=file:components.*deps+content&sq=package:chromium&type=cs

[Eugene But]

I added blundel@ to this thread, who designed //components. I think he would be a good person to provide details on the allowed dependencies. 

[Eric Seckler]

Wanted to add that even when the net service runs in-process, its tasks execute in a separate "net service thread" in the browser, not on the IO thread. That's why even on low-end Android, it didn't seem like scheduling on the IO thread was really necessary. (If the IO thread is only used for receiving mojo messages from other processes (+sending legacy IPCs), there isn't much left to schedule.) Things could have changed though.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CACiDxEFshLORryY%2BWh4_gBvkUJhn%3DUfhS_ju-%2BisYJw8JXG9tg%40mail.gmail.com.

[Kinuko Yasuda]

Just to add more context, for many other IO thread tasks (like navigations, service workers, indexed db etc) there's a plan to move them out of the IO thread because after mojofication & network service there's no much need to run them there (but just run them on UI thread or on their own task runners).

(Separately from that having a scheduler for net service tasks could be probably still good as weve tended to see a lot of task scheduling issues there)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CAHZJZiFb4Hka%2B0a_m1oJYWCuB%2BGCpAQ5BR66BtfUKKeG8MTDQg%40mail.gmail.com.

[Colin Blundell]

Responses inline, thanks!

On Sat, Feb 16, 2019 at 12:44 AM Nico Weber <tha...@chromium.org> wrote:

I thought that used to be true long ago but isn't any more: https://cs.chromium.org/search/?q=file:components.*deps+content&sq=package:chromium&type=cs

 

Nico, was this the response that you were intending to make? Eugene's point was that //components isn't a place to share code between //content and //ios/web, not that individual components themselves can't depend on //content.

If your point was instead that //content now depends on //components, that is both true and in my opinion a bug. It was introduced to share code between Blink and Mandoline when Mandoline existed. It looks like it's persisted as a mechanism for sharing low-level features between Blink and //content/browser. I would rather see us have a different location for such features.

 

On Fri, Feb 15, 2019 at 6:14 PM Eugene But <euge...@chromium.org> wrote:

By design //components layer sits above //content and //ios/web. Higher layer can use the code from lower layer:

chrome

components

content

net

base

On Fri, Feb 15, 2019 at 3:11 PM Gabriel Charette <g...@chromium.org> wrote:

On Fri, Feb 15, 2019 at 12:30 PM Eugene But <euge...@chromium.org> wrote:

Hi François,

It would be really amazing if we could move threading code out of //ios/web. Web Layer should be responsible for managing web contents and navigation, and threading support is out of scope for this layer. There are a few constraints however:

  - //content and //ios/web can not use //components (existing examples are layering violations)

  - //ios/web can not use //content (which is currently true)

//content and //ios/web can use the code from //base. So maybe we could move all shared code to //base?

Adding thankis@ for evaluating if //base is the right place for threading code. blundell@ who designed layered components. rohitrao@ who is probably the only engineer on iOS team who understands //ios/web threading code.

No //base isn't the right place for BrowserThreads. Why can't we have a basic component for this? I thought this was the whole point of components but I keep being confused at why components can't be used in various places. Feels against base's principles of not being a dumpground to ban extracting a common component from //content and //ios/web (which are currently out-of-phase copies of each other..).

//components started off as high-level browser features usable by multiple embedders of //content. When we unforked iOS, it then became browser features usable by multiple embedders, with individual components determining the model of their dependence on //content based on sharing on iOS. As time has gone on, the purpose of //components and whether something belongs there or not has gotten more and more murky. As I wrote above, I think that the fact that //content is now allowed to depend on //components is a large part of the cause of this. I would be very reluctant to go further in that direction. However, the fact that //content can now depend on //components does make it theoretically possible to imagine relaxing this constraint. I would be open to having a discussion about the pros and cons of changing this policy. I acknowledge that the current layering of the codebase doesn't provide easy answers to "I want to share code between //content and //ios/web."

Best,

Colin

[François Doray]

(Note: The discussion on having an IO thread scheduler should move to a different email thread.)

These are "components" on which both //content" and //ios/web depend:

• //url/
• //mojo
• //crypto
• //components/url_formatter

These are "components" on which only "//content" depends:

• components/discardable_memory
• components/download
• components/filename_generation
• components/services/font
• components/services/filesystem
• components/services/font
• components/services/leveldb
• components/session_manager
• components/link_header_util
• components/metrics
• components/network_session_configurator
• components/offline_pages
• components/payments
• components/profile_service
• components/rappor
• components/tracing
• components/ukm
• components/url_formatter
• components/viz
• components/cbor

What do you think of moving all of the above to a new top-level directory called //browser_base? Once this is done, the "browser threads" functionality provided by duplicated code in //content and //ios/web can move there as well, and we can really enforce that //content doesn't depend on //components? (which would help developers understand the purpose of //components).

[Colin Blundell]

Hi,

Thanks for this analysis! There has historically been resistance to creating a second "//components-like" directory. However, I think that the arguments in favor of it are stronger than they ever were before. Can you put together a short proposal for what you're suggesting (I would leave out //url, //mojo, and //crypto for the purposes of the proposal, as I think that would just muddy the waters and at the current time everyone can use those anyway) and share it with me for an initial lookover/editing pass? I'll then start a discussion with relevant folks (//components and top-level OWNERS plus the relevant folks on this thread) to test the waters. Don't be surprised if the resistance is still there though :).

Best,

Colin

[Gabriel Charette]

Great analysis François! +1 to browser_base

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CAMGE5NHYE-vtGyCDKP0VTuK_OyOcLbqu7O66tBGaMUBu8LUQ%2Bg%40mail.gmail.com.

[Gabriel Charette]

FWIW, I just had to dig into history for this. Turns out the biggest discrepancy between //ios/web and //content -- and reason I thought we couldn't simply migrate to a common component and drop the iOS lage -- was https://chromium-review.googlesource.com/c/chromium/src/+/980793 but +Rohit Rao closed that gap in https://chromium-review.googlesource.com/c/chromium/src/+/1383140. So now iOS is only missing https://chromium-review.googlesource.com/973556 and as such it's susceptible to a bug fixed long ago in //content : BrowserThread-lock-contention and priority inversions (i.e. crbug.com/821034 and crbug.com/890978).

Fixing that alone (and making sure to share further such improvements -- i.e. BrowserThreadUIScheduler which just landed in //content) is enough to justify //browser_base IMO.

The iOS bug tracking the required catch up is crbug.com/826465 but, again, I think sharing code would be a much better end-state for iOS which keeps missing out on threading improvements. As such I also think that the iOS team should own implementing this (and probably share the design duty with fdoray@?).

[Colin Blundell]

On Wed, Feb 20, 2019 at 5:26 PM Gabriel Charette <g...@chromium.org> wrote:

FWIW, I just had to dig into history for this. Turns out the biggest discrepancy between //ios/web and //content -- and reason I thought we couldn't simply migrate to a common component and drop the iOS lage -- was https://chromium-review.googlesource.com/c/chromium/src/+/980793 but +Rohit Rao closed that gap in https://chromium-review.googlesource.com/c/chromium/src/+/1383140. So now iOS is only missing https://chromium-review.googlesource.com/973556 and as such it's susceptible to a bug fixed long ago in //content : BrowserThread-lock-contention and priority inversions (i.e. crbug.com/821034 and crbug.com/890978).

Fixing that alone (and making sure to share further such improvements -- i.e. BrowserThreadUIScheduler which just landed in //content) is enough to justify //browser_base IMO.

Thanks! This should definitely go in the doc that we discussed below.

 

The iOS bug tracking the required catch up is crbug.com/826465 but, again, I think sharing code would be a much better end-state for iOS which keeps missing out on threading improvements. As such I also think that the iOS team should own implementing this (and probably share the design duty with fdoray@?).

Before getting into who would implement what, I think that we should first focus on having the design discussion with the relevant top-level OWNERS who would need to approve the addition of a new top-level directory in any case. I'm happy to collaborate with fdoray@ to spark that discussion.

Best,

Colin