697830 - Delayed DNS prefetch queue is disabled for e10s

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Steve Workman [:sworkman] (INACTIVE)]

From bug 565163 comment 17: This patch "effectively disables the delayed prefetch queue, so gives a big hit on DOM manipulation performance."

e10s code in nsHTMLDNSPrefetch bypasses the deferrals queue and calls sDNSService->AsyncResolve directly.  This most likely results in a performance hit for DOM manipulation, specifically for nsHTMLAnchorElements.

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

Bz, can you provided some more information, please.  In bug 565163 comment 17 I'm not sure which other comment about IPC you're referencing.  Can you be more explicit please, and explain why it's bogus?

Also, I was looking at bug 464838 which added the deferrals queue.  In bug 464838, comment 5 you mention that mLock is not needed.  Subsequent patches in that bug then have a check to ensure the deferrals methods are called on the main thread.  I've looked through and I think I'm missing information:
-- Why is the deferrals queue processing only on the main thread?
-- Why is a mutex not needed for the add function

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm talking about this comment: http://hg.mozilla.org/mozilla-central/file/ccfa9105af73/content/html/content/src/nsHTMLDNSPrefetch.cpp#l144

It's bogus because it's comparing the cost of the GetHostname to the cost of the IPC call, whereas the right comparison is the cost of the GetHostname+IPC call to the cost of adding the element to the prefetch queue and doing nothing with it after that, which is what happens with DOM manipulation situations.

> -- Why is the deferrals queue processing only on the main thread?

Because it's done off a timer notification which was posted on the main thread and hence is delivered on the main thread.

> -- Why is a mutex not needed for the add function

Because at the moment it's only called on the main thread.  It should probably be asserting that.

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Using XUL-based image on Samsung Gal Tab 10.1, I have the following results for a recent patch (to be uploaded next) using dromaeo dom-modify.

TEST            PATCH   BASE
createElement:  36.43   37.67
createTextNode:  6.37    6.39
innerHTML:      16.59   10.39 *
cloneNode:      12.18   12.30
appendChild:    91.19   27.78 *
insertBefore:   55.75   23.89 *

Values are in "runs/s".  According to these results, the patch show improvement for innerHTML, appendChild and insertBefore.  The other tests look similar.  So, it looks like it does make a difference for e10s.

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 diff

Prefetch requests coming from nsHTMLAnchorElement in e10s are added to a deferral queue, just as in the non-e10s case.

Comment 5

[Jason Duell]

Comment on attachment 585102 [details] [diff] [review]
v1.0 diff

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

bz: feel free to look at this too if you want, but this doesn't need 2 +r's or an +sr, and it seems pretty clear that we're doing what you wanted (deferring e10s DNS prefetch just like non-e10s).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yep.  Patch looks good!

Comment 7

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/754c87545661

Comment 8

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/mozilla-central/rev/754c87545661

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

Thanks for the push to m-i, m-c!

Comment 10

[Boris Zbarsky [:bzbarsky]]

Steve, did you mean to reopen?

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

No. I just saw that. Sigh. Bugzilla has done this to me before. Sorry, I'll fix it now.