Delayed DNS prefetch queue is disabled for e10s

RESOLVED FIXED in mozilla12

Status

()

Core
Networking
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sworkman, Assigned: sworkman)

Tracking

10 Branch
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
(Assignee)

Comment 1

6 years ago
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
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.
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
Created attachment 585102 [details] [diff] [review]
v1.0 diff

Prefetch requests coming from nsHTMLAnchorElement in e10s are added to a deferral queue, just as in the non-e10s case.
Attachment #585102 - Flags: review?(jduell.mcbugs)
Attachment #585102 - Flags: review?(bzbarsky)
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).
Attachment #585102 - Flags: review?(jduell.mcbugs)
Attachment #585102 - Flags: review?(bzbarsky)
Attachment #585102 - Flags: review+
Yep.  Patch looks good!
https://hg.mozilla.org/integration/mozilla-inbound/rev/754c87545661
https://hg.mozilla.org/mozilla-central/rev/754c87545661
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Target Milestone: --- → mozilla12
(Assignee)

Comment 9

6 years ago
Thanks for the push to m-i, m-c!
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
Target Milestone: mozilla12 → ---
Steve, did you mean to reopen?
No. I just saw that. Sigh. Bugzilla has done this to me before. Sorry, I'll fix it now.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.