Last Comment Bug 660959 - Figure out a better setup for figuring out link state
: Figure out a better setup for figuring out link state
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
-- normal (vote)
: mozilla11
Assigned To: David Zbarsky (:dzbarsky)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 598833
Blocks: 631527 691195
  Show dependency treegraph
 
Reported: 2011-05-31 13:33 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-02-01 13:58 PST (History)
8 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Update link state before resolving style (2.40 KB, patch)
2011-06-23 12:40 PDT, David Zbarsky (:dzbarsky)
bzbarsky: review-
Details | Diff | Splinter Review
Update link state less lazily (9.76 KB, patch)
2011-06-27 06:50 PDT, David Zbarsky (:dzbarsky)
bzbarsky: review+
Details | Diff | Splinter Review
Patch to check in r=bz (9.77 KB, patch)
2011-06-27 17:33 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch (9.77 KB, patch)
2011-06-28 14:13 PDT, David Zbarsky (:dzbarsky)
bzbarsky: review+
Details | Diff | Splinter Review
Patch (23.72 KB, patch)
2011-06-29 16:54 PDT, David Zbarsky (:dzbarsky)
bzbarsky: review+
Details | Diff | Splinter Review
Patch to check in r=bz (23.77 KB, patch)
2011-06-30 10:08 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch (25.23 KB, patch)
2011-07-12 15:35 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch (25.54 KB, patch)
2011-07-14 11:16 PDT, David Zbarsky (:dzbarsky)
bzbarsky: review+
Details | Diff | Splinter Review
Patch r=bz (34.44 KB, patch)
2011-07-18 22:28 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch r=bz (35.57 KB, patch)
2011-07-19 09:49 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch r=bz (35.62 KB, patch)
2011-07-21 14:26 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch r=bz (35.69 KB, patch)
2011-08-10 11:55 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Same thing, updated to tip so it can actually land (34.57 KB, patch)
2011-11-13 19:23 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review

Description User image Boris Zbarsky [:bz] (still a bit busy) 2011-05-31 13:33:41 PDT
It's expensive, so we do it lazily.... the problem is when.  Right now we have hacks in style resolution to trigger it, basically, but that's not so hot if we want to parallelize.  Can we do better?
Comment 1 User image David Zbarsky (:dzbarsky) 2011-06-23 12:40:50 PDT
Created attachment 541461 [details] [diff] [review]
Update link state before resolving style
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2011-06-23 14:11:19 PDT
Comment on attachment 541461 [details] [diff] [review]
Update link state before resolving style

Does this not break querySelector?

What about resolutions coming via ReResolveStyleContext?

I'm pretty sure this change is wrong....
Comment 3 User image David Zbarsky (:dzbarsky) 2011-06-27 06:50:51 PDT
Created attachment 542144 [details] [diff] [review]
Update link state less lazily
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2011-06-27 14:31:31 PDT
Comment on attachment 542144 [details] [diff] [review]
Update link state less lazily

Hmm.  I think this should work, but can you add some of the tests your initial patch would have failed, please?

And fix the commit message!
Comment 5 User image David Zbarsky (:dzbarsky) 2011-06-27 16:28:27 PDT
This patch does have tests.  Do you mean some other tests?
Comment 6 User image David Zbarsky (:dzbarsky) 2011-06-27 17:33:25 PDT
Created attachment 542339 [details] [diff] [review]
Patch to check in r=bz
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2011-06-27 19:17:33 PDT
Comment on attachment 542339 [details] [diff] [review]
Patch to check in r=bz

Er, I somehow completely missed the tests, sorry.

The first test is wrong.  It needs to do $("content").querySelector, and ideally put an id on the <a> and make sure querySelector returned the right thing.  So something like:

  <div id="content" style="display: none">
    <a href="#" id="testa"></a>
  </div>

  <script>
    is($("content").querySelector(":link, :visited"), $("testa"),
       "Should find a link even in a display:none subtree");
  </script>

The second test is fine, though you can replace ".getPropertyValue('color').toString()" with ".color" there.
Comment 8 User image David Zbarsky (:dzbarsky) 2011-06-28 14:13:04 PDT
Created attachment 542592 [details] [diff] [review]
Patch
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2011-06-28 20:59:35 PDT
Comment on attachment 542592 [details] [diff] [review]
Patch

r=me
Comment 10 User image David Zbarsky (:dzbarsky) 2011-06-29 16:54:26 PDT
Created attachment 543006 [details] [diff] [review]
Patch

This one doesn't have a slowdown on the selector test
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2011-06-29 22:39:55 PDT
Comment on attachment 543006 [details] [diff] [review]
Patch

I just realized that the nsGlobalWindow change is wrong.  You should QI to nsIContent, then check that it's non-null and IsElement() and then call AsElement().... r=me with that.
Comment 12 User image David Zbarsky (:dzbarsky) 2011-06-30 10:08:53 PDT
Created attachment 543170 [details] [diff] [review]
Patch to check in r=bz
Comment 13 User image Boris Zbarsky [:bz] (still a bit busy) 2011-07-05 08:48:01 PDT
Actually, I think there's still a problem here...  Consider a display:none subtree containing this markup:

  <a href="http://www.example.com"></a>
  <div id="foo">
    <span></span>
  </div>

And a call to document.getElementById("foo").querySelector(":link + * span, :visited + * span").

This should return the span.  I bet with your changes it doesn't....
Comment 14 User image David Zbarsky (:dzbarsky) 2011-07-12 15:35:34 PDT
Created attachment 545507 [details] [diff] [review]
Patch
Comment 15 User image Boris Zbarsky [:bz] (still a bit busy) 2011-07-12 20:15:29 PDT
Two comments offhand:

1)  Why not just use the hashtable as a member, instead of pointer-to-hashtable?
    This hashtable will be used on basically every page....
2)  You probably need to either have a scriptblocker in FlushPendingLinkUpdates
    or stick all the element pointers into an array before making the
    RequestLinkStateUpdate calls, because I don't quite trust those calls to not
    add/remove things in this hashtable.  Scriptblocker is probably simpler.

I'll look through the rest carefully tomorrow.
Comment 16 User image David Zbarsky (:dzbarsky) 2011-07-14 11:16:38 PDT
Created attachment 545959 [details] [diff] [review]
Patch

Dromaeo DOM results

Before patch:
    appendChild: 653.94runs/s ±0.89%
    insertBefore: 442.44runs/s ±1.46%

After patch:
    appendChild: 643.07runs/s ±0.99%
    insertBefore: 437.16runs/s ±2.30%
Comment 17 User image Boris Zbarsky [:bz] (still a bit busy) 2011-07-18 12:25:43 PDT
Comment on attachment 545959 [details] [diff] [review]
Patch

Please document your new methods and member.  

I don't understand the block at the beginning of UnregisterPendingLinkUpdate.  Why is it there?

In UnbindFromTree, you can set doc to GetCurrentDoc so you only unregister when you might possibly be registered.

To address the Element precondition thinkg you added, do you want to just switch the document hashtable to using Link instead of Element?  That seems like it would be cleaner...

r=me with those changes.  Thank you!
Comment 18 User image David Zbarsky (:dzbarsky) 2011-07-18 22:28:19 PDT
Created attachment 546711 [details] [diff] [review]
Patch r=bz

> I don't understand the block at the beginning of UnregisterPendingLinkUpdate.  > Why is it there?

I removed the Get call. There's no point in performing the hash lookup if there are no elements in the table.
Comment 19 User image Boris Zbarsky [:bz] (still a bit busy) 2011-07-19 06:42:05 PDT
That last patch still needs comments.  It's also missing the changes to Link.h and Link.cpp, and the nsDocument code is still trying to call RequestLinkStateUpdate on Element.  Did you forget to qref somewhere?
Comment 20 User image David Zbarsky (:dzbarsky) 2011-07-19 09:49:48 PDT
Created attachment 546814 [details] [diff] [review]
Patch r=bz

This time with comments
Comment 21 User image Boris Zbarsky [:bz] (still a bit busy) 2011-07-19 10:31:24 PDT
It's too bad that you can't call the method Element().  At least document that it can't return null?
Comment 22 User image David Zbarsky (:dzbarsky) 2011-07-21 14:26:25 PDT
Created attachment 547517 [details] [diff] [review]
Patch r=bz
Comment 23 User image David Zbarsky (:dzbarsky) 2011-08-10 11:55:30 PDT
Created attachment 552155 [details] [diff] [review]
Patch r=bz
Comment 24 User image Boris Zbarsky [:bz] (still a bit busy) 2011-11-13 19:23:11 PST
Created attachment 574221 [details] [diff] [review]
Same thing, updated to tip so it can actually land
Comment 25 User image Boris Zbarsky [:bz] (still a bit busy) 2011-11-14 02:15:58 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/61094ae6da18
Comment 26 User image Ed Morley [:emorley] 2011-11-14 19:38:15 PST
https://hg.mozilla.org/mozilla-central/rev/61094ae6da18

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