Closed Bug 1356556 Opened 3 years ago Closed 3 years ago

nsHTMLDNSPrefetch shouldn't create weak references all the time

Categories

(Core :: DOM: Core & HTML, enhancement)

50 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files, 3 obsolete files)

Need to find some faster solution, so that BindToTree isn't so slow for links.
Blocks: 535638
Hmm, that patch can't be enough. Need to check more than IsInComposedDoc
Attachment #8858314 - Attachment is obsolete: true
Attached patch Add the elements to CC (obsolete) — Splinter Review
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)
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)
Attached patch link_dns_opt.diff (obsolete) — Splinter Review
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-
-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
(Note the existing flags for handling deferred prefetch don't capture what we need here.)
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+
Added comment and improved commit message
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
https://hg.mozilla.org/mozilla-central/rev/03cdd7137ef1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.