1295565 - network predictor can spam the parent process main thread during load

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1231222 we are trying to move service worker interception from the child process to the parent process.  We discovered, however, that this led to large perf regressions.  Digging into the problem showed that the main thread in both the child and parent are quite busy during page load.

Surprisingly, net::Predictor::Learn() takes up ~125ms on the parent main thread in this work load.

Search for "Predictor::Learn" in this profile:

https://cleopatra.io/#report=8b52aa6bdb297f4e7389b02931638a00048eef69

This is while loading this site:

https://jakearchibald.github.io/service-worker-benchmark/

Would it be possible to defer Predictor::Learn work until after the page's load event has been fired?  Or is there a way to speed up the predictor work so it doesn't take so long?  Right now it does not seem to scale very well.

Comment 1

[u408661]

So taking a look at the profile, although I'm only seeing Predictor::Learn taking 54ms in the profile (I could be reading it wrong), there's definitely some sub-optimal behavior that can be cleaned up. Most of the time in Learn is taken up by creating new URIs, and a lot of those URIs we don't actually need to create (they're not used for comparing URI equality, and so can stay strings - we literally just round-trip from AsciiSpec->URI->AsciiSpec). That round-tripping accounts for about 65% of the time used by Predictor::Learn, so removing it can cut 54ms to 19ms, or 125ms to 44ms.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

That would be great!

But, is there a reason we need to do this work while we are loading the page?  Is the result used within the current page load?  Or does it only affect future page loads?

Comment 3

[u408661]

Delaying the work until after pageload completes would require a significant amount of reworking - creating some kind of "transaction" equivalent in the predictor, making sure it gets created and committed or rolled back at the appropriate times, making sure the buffering of information in the transaction doesn't take up a significant amount of memory, and probably a whole host of other things I'm not thinking of right now.

If you add in the fact that multiple tabs could be loading at once, and thus multiple transactions would need to be created and scheduled such that they interfere with each other as little as possible (page 'a' finishes loading, commits its transaction, which then messes with page 'b' loading in another tab in the same manner a page messes with itself now, possibly worse), the amount of work required to implement this is looking even more and more blue sky-ish.

Basically, we're going to get a pretty good bang for our buck removing the URI round trips, and while the transaction idea might be nice in the future, I'd be willing to bet there are other low(er)-hanging fruit bits elsewhere in gecko to save main thread time.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

So it sounds like we're using some kind of in-flight state in the http cache to coordinate between the "learn" message and the end resulting request?  If so, then yea it does seem hard to do what I am asking.

Another question:

Should we be sending these Learn() messages before we know if its actually going to go to the network?  In this particular profile the requests are all intercepted by the service worker and do not hit the network.

Comment 5

[u408661]

Whether to ship across Learn()s for items intercepted by the service worker or not is really a question of how likely it is that those items would not be intercepted by the sw in the future. If there's no chance they wouldn't be intercepted, simple! If there is a chance, then the calculus is not so simple - what chance is it? How often does it not get intercepted? Where's a good cutoff? Etc, etc.

However, we don't want to only dispatch Learn() when it hits the network - we definitely still want to Learn() items that are served up from the cache, for example, since it's very possible that those items could be evicted from the cache at any time.

Comment 6

[u408661]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75da2a10b524

Comment 7

[u408661]

Attachment: Bug 1295565 - Don't create URIs if we don't have to.

Review commit: https://reviewboard.mozilla.org/r/73836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73836/

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8784359 [details]
Bug 1295565 - Don't create URIs if we don't have to.

https://reviewboard.mozilla.org/r/73836/#review72382

::: netwerk/base/Predictor.cpp:1696
(Diff revision 1)
>    if (!IsURIMetadataElement(key)) {
>      // This isn't a bit of metadata we care about
>      return NS_OK;
>    }
>  
>    nsCOMPtr<nsIURI> parsedURI;

looks unused anymore?

::: netwerk/base/Predictor.cpp:1709
(Diff revision 1)
>      nsKey.AssignASCII(key);
>      mLongKeysToDelete.AppendElement(nsKey);
>      return NS_OK;
>    }
>  
> -  nsCString uri;
> +  nsCString uri(key + (sizeof(META_DATA_PREFIX) - 1));

Is it possible to use nsDependentCSubstring here?  Substring function may help even more here.

or simply uriLength = key.Length() - sizeof(..) ?

Comment 9

[u408661]

Comment on attachment 8784359 [details]
Bug 1295565 - Don't create URIs if we don't have to.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73836/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ee0ea599ba3
Don't create URIs if we don't have to. r=mayhemer

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ee0ea599ba3