Last Comment Bug 697830 - Delayed DNS prefetch queue is disabled for e10s
: Delayed DNS prefetch queue is disabled for e10s
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: 10 Branch
: All All
: -- normal (vote)
: mozilla12
Assigned To: Steve Workman [:sworkman] (INACTIVE)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-27 14:21 PDT by Steve Workman [:sworkman] (INACTIVE)
Modified: 2012-01-17 15:47 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1.0 diff (2.17 KB, patch)
2011-12-30 19:24 PST, Steve Workman [:sworkman] (INACTIVE)
jduell.mcbugs: review+
Details | Diff | Splinter Review

Description Steve Workman [:sworkman] (INACTIVE) 2011-10-27 14:21:08 PDT
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) 2011-10-27 14:21:30 PDT
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 [:bz] 2011-10-27 14:34:57 PDT
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) 2011-12-30 19:16:12 PST
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) 2011-12-30 19:24:30 PST
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.
Comment 5 Jason Duell [:jduell] (needinfo me) 2012-01-12 20:20:58 PST
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 [:bz] 2012-01-12 20:54:13 PST
Yep.  Patch looks good!
Comment 7 Jason Duell [:jduell] (needinfo me) 2012-01-13 22:26:32 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/754c87545661
Comment 8 Jonathan Kew (:jfkthame) 2012-01-16 04:59:59 PST
https://hg.mozilla.org/mozilla-central/rev/754c87545661
Comment 9 Steve Workman [:sworkman] (INACTIVE) 2012-01-17 14:57:42 PST
Thanks for the push to m-i, m-c!
Comment 10 Boris Zbarsky [:bz] 2012-01-17 15:09:59 PST
Steve, did you mean to reopen?
Comment 11 Steve Workman [:sworkman] (INACTIVE) 2012-01-17 15:47:37 PST
No. I just saw that. Sigh. Bugzilla has done this to me before. Sorry, I'll fix it now.

Note You need to log in before you can comment on or make changes to this bug.