1213443 - Allow more parallelism for <link rel="prefetch">

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

In bug 1172205 comment 28 it was pointed out that we apparently serialize prefetches from <link rel="prefetch">.  Can we parallelize them somewhat?

Comment 1

[u408661]

Over in bug 1172205 comment 32, I noted that it seems possible to do by swapping a pointer for a list of pointers (that description is intentionally vague, given my unfamiliarity with the code) in nsPrefetchService. I haven't actually tried to make it happen in practice, though.

Comment 2

[u408661]

Attachment: Parallelism for <link rel=prefetch>

This does my "simple" change of swapping a pointer for an array of
nsRefPtrs. The one bit that doesn't exactly preserve current behavior is
the removal of the iterator for nsPrefetchService - it wasn't used anywhere
but in one test, and the reason that test used the iterator is more simply
satisfied without having to have an entire (otherwise unused) iterator
class.

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8675046 [details] [diff] [review]
Parallelism for <link rel=prefetch>

>+nsPrefetchService::ProcessNextURI(nsPrefetchNode *aFinished)
>+        int32_t idx = mCurrentNodes.IndexOf(aFinished);
>+        if (idx >= 0) {
>+            mCurrentNodes.RemoveElementAt(idx);

  mCurrentNodes.RemoveElement(aFinished);

?

> nsPrefetchService::StopPrefetching()
>     // only kill the prefetch queue if we've actually started prefetching.
>-    if (!mCurrentNode)
>+    if (mHaveProcessed) {

This change isn't making sense to me.  Why shouldn't we clear queues if mHaveProcessed is true?


r=me on the rest, but would like to understand what StopPrefetching is doing there.

Comment 4

[u408661]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Comment on attachment 8675046 [details] [diff] [review]
> Parallelism for <link rel=prefetch>
> 
> >+nsPrefetchService::ProcessNextURI(nsPrefetchNode *aFinished)
> >+        int32_t idx = mCurrentNodes.IndexOf(aFinished);
> >+        if (idx >= 0) {
> >+            mCurrentNodes.RemoveElementAt(idx);
> 
>   mCurrentNodes.RemoveElement(aFinished);
> 
> ?

*facepalm* fixed

> > nsPrefetchService::StopPrefetching()
> >     // only kill the prefetch queue if we've actually started prefetching.
> >-    if (!mCurrentNode)
> >+    if (mHaveProcessed) {
> 
> This change isn't making sense to me.  Why shouldn't we clear queues if
> mHaveProcessed is true?
> 
> 
> r=me on the rest, but would like to understand what StopPrefetching is doing
> there.

So for the most part (in my limited understanding of this wonderfully old and unmaintained code) it seems like StopPrefetching/StartPrefetching are used similarly to wait/signal on a semaphore to prevent prefetching from happening until the rest of the pageload has completed - I think iframes in the load also cause StopPrefetching to be called, so you could easily have a mStopCount count > 1. However, it's the case that the HTML parser may have put something in the queue before an iframe (or other resource?) calls StopPrefetching, and we don't want to clear out the queue in that case (since that would lose pending prefetches that are totally legitimate). Once mStopCount eventually reaches 0 (because the page is fully loaded), I think the only time StopPrefetching would be called is when the page is unloaded, so it makes sense to remove any pending prefetches at that point.

The old code was using mCurrentNode to determine if mStopCount had ever reached 0 (mCurrentNode would always point to some node, possibly a freed one, once prefetching had started). mHaveProcessed is true in the same situations when mCurrentNode would be non-null, so it's a good proxy for the old behavior (without holding a pointer to freed memory around). Why the old code didn't use mHaveProcessed, I have no idea - it's safer, and results in exactly the same behavior as !mCurrentNode.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> mHaveProcessed is true in the same situations when mCurrentNode would be non-null

It is?  I don't see us setting it to false in StopPrefetching or ProcessNextURI.  Am I just missing something?

Comment 6

[u408661]

(In reply to Boris Zbarsky [:bz] from comment #5)
> > mHaveProcessed is true in the same situations when mCurrentNode would be non-null
> 
> It is?  I don't see us setting it to false in StopPrefetching or
> ProcessNextURI.  Am I just missing something?

Argh, no, you're right. I'll figure out a new, better proxy for the old !mCurrentNode behavior, and update the patch with that.

Comment 7

[u408661]

OK, I looked through it again, and I shouldn't have trusted (my interpretation of?) the comment just above that line - it has nothing to do with whether we've started prefetching at all in the past, and everything to do with whether we are actively prefetching something *right now*. So, it seems that instead of |if (mHaveProcessed)| it should just be |if (!mCurrentNodes.Length())|

I've made that change, and updated the comment to be more reflective of reality.

bz, I'm happy to upload a new patch, or if you're fine with that change as described, I'll go ahead and land.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> So, it seems that instead of |if (mHaveProcessed)| it should just be |if
> (!mCurrentNodes.Length())|

Yes, that I buy.  r=me with that change and a fix to the comment.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky [:bz] from comment #8)
> > So, it seems that instead of |if (mHaveProcessed)| it should just be |if
> > (!mCurrentNodes.Length())|
> 
> Yes, that I buy.  r=me with that change and a fix to the comment.

Can I vote for mCurrentNodes.IsEmpty() ?

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f3133b70d2

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a0f3133b70d2

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a0f3133b70d2