nsHTMLDNSPrefetch shouldn't create weak references all the time

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

50 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

9 months ago
Need to find some faster solution, so that BindToTree isn't so slow for links.
(Assignee)

Updated

9 months ago
Blocks: 535638
(Assignee)

Comment 2

9 months ago
Hmm, that patch can't be enough. Need to check more than IsInComposedDoc
(Assignee)

Updated

9 months ago
Attachment #8858314 - Attachment is obsolete: true
(Assignee)

Comment 3

9 months ago
Created attachment 8858404 [details] [diff] [review]
Add the elements to CC

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


This is perhaps a bit annoying to review, but the idea is that there is global list of elements (in sPrefetches) and each document, which may have elements there, traverses/unlinks the relevant elements.
The flag in nsIDocument is to optimize out unneeded traversing.
Attachment #8858404 - Flags: review?(continuation)
(Assignee)

Comment 4

9 months ago
Comment on attachment 8858404 [details] [diff] [review]
Add the elements to CC

hmm, no, I'm still not happy. This makes us not leak, but elements may keep other things alive too long.
Attachment #8858404 - Flags: review?(continuation)
(Assignee)

Comment 5

9 months ago
Created attachment 8858460 [details] [diff] [review]
link_dns_opt.diff

Since this is on top of the other Link related patch, asking review from you bz.
Feel free to forward to someone else, perhaps peterv.

This is perhaps a bit controversial because of RemoveUnboundLinks handling, but in practice should behave rather well. SnowWhiteKiller after all kills objects in batches.
And avoiding the extra malloc WeakRef needs for Elements makes quite some difference in performance.

Hopefully try likes this.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35ff835b26e573e22f2f9300271022de948181b6
Attachment #8858460 - Flags: review?(bzbarsky)
Comment on attachment 8858460 [details] [diff] [review]
link_dns_opt.diff

This really needs a commit message describing the setup...
Attachment #8858460 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 7

9 months ago
Created attachment 8858514 [details] [diff] [review]
link_dns_opt_3.diff

-m "Bug 1356556, nsHTMLDNSPrefetch can keep raw pointers to Links since the destructor of the Link clears the raw pointer, r=bz"



That is the idea, and to optimize out going through the array for each Link dtor call, RemoveUnboundLinks is called and that removes all that once.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6406d124311a4b549ccc164c969e8efcb91358f9
Attachment #8858404 - Attachment is obsolete: true
Attachment #8858460 - Attachment is obsolete: true
(Assignee)

Comment 8

9 months ago
(Note the existing flags for handling deferred prefetch don't capture what we need here.)
(Assignee)

Comment 9

9 months ago
Comment on attachment 8858514 [details] [diff] [review]
link_dns_opt_3.diff

See comment 7.
Attachment #8858514 - Flags: review?(bzbarsky)
Comment on attachment 8858514 [details] [diff] [review]
link_dns_opt_3.diff

> That is the idea, and to optimize out going through the array for each Link dtor call, RemoveUnboundLinks is called and that removes all that once.

I'd really appreciate the commit message, explaining this.  Or maybe the code comment in nsHTMLDNSPrefetch::LinkDestroyed could be expanded to something about how all links that are not in a composed doc can be cleared out of the list, and we're just using the opportunity we have here to do so.

The aLink->IsInDNSPrefetch() check should probably happen before the LinkDestroyed() call, so we don't bother with the function call if not needed.

r=me with that.
Attachment #8858514 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 11

9 months ago
Created attachment 8859523 [details] [diff] [review]
link_dns_opt_4.diff

Added comment and improved commit message

Comment 12

9 months ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03cdd7137ef1
use of weak references in nsHTMLDNSPrefetch is malloc heavy, so instead use raw pointers and a flag to tell that Link's destructor needs to clear the raw pointer, r=bz

Comment 13

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/03cdd7137ef1
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.