Last Comment Bug 762345 - Skip all the QIing stuff in the TreeMatchContext constructor if possible
: Skip all the QIing stuff in the TreeMatchContext constructor if possible
Status: RESOLVED FIXED
[qa-]
: perf, regression
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on:
Blocks: 722853
  Show dependency treegraph
 
Reported: 2012-06-06 19:36 PDT by Boris Zbarsky [:bz]
Modified: 2012-08-14 08:14 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed


Attachments
Patch (v1) (1.40 KB, patch)
2012-06-07 12:20 PDT, :Ehsan Akhgari
bzbarsky: review-
Details | Diff | Splinter Review
Patch (v2) (1.40 KB, patch)
2012-06-07 12:43 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review
Patch (v3) (3.26 KB, patch)
2012-06-07 20:24 PDT, :Ehsan Akhgari
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2012-06-06 19:36:24 PDT
This was added in bug 722853 and it's showing up in querySelector profiles.

Can we not skip all this stuff based on our visited handling value?
Comment 1 :Ehsan Akhgari 2012-06-07 08:43:45 PDT
What's the exact semantics of mVisitedHandling?
Comment 2 Boris Zbarsky [:bz] 2012-06-07 08:58:06 PDT
That's the part I can't recall exactly....

But for the case of querySelector, the semantic is "treat all links as unvisited".
Comment 3 :Ehsan Akhgari 2012-06-07 10:14:38 PDT
Does that only happen when we're in PB mode then?  Or can it also be on as a result of a pref being set, etc.?
Comment 4 Boris Zbarsky [:bz] 2012-06-07 10:29:35 PDT
What do you mean?

The point is that querySelector(":visited") will always return null, no matter what's going on with PB mode.  So there's no point checking for the PB mode stuff under querySelector, since it always treats all links as unvisited anyway, no matter what.
Comment 5 :Ehsan Akhgari 2012-06-07 12:20:22 PDT
Created attachment 631078 [details] [diff] [review]
Patch (v1)

Like this?
Comment 6 Boris Zbarsky [:bz] 2012-06-07 12:28:52 PDT
Comment on attachment 631078 [details] [diff] [review]
Patch (v1)

The if check is backwards, no?
Comment 7 :Ehsan Akhgari 2012-06-07 12:43:06 PDT
Created attachment 631087 [details] [diff] [review]
Patch (v2)

Indeed it is!  :-)

Which makes me think, how should I test this patch?  I ran the PB tests and the layout/style tests and everything seemed to be fine.  I guess I'll push it to try.
Comment 8 :Ehsan Akhgari 2012-06-07 12:44:56 PDT
https://tbpl.mozilla.org/?tree=Try&rev=d5514760a513
Comment 9 Boris Zbarsky [:bz] 2012-06-07 13:31:46 PDT
Comment on attachment 631087 [details] [diff] [review]
Patch (v2)

r=me
Comment 10 :Ehsan Akhgari 2012-06-07 13:36:39 PDT
Hmm, this patch is not correct, it fails a test that we have for exactly this reason:

https://tbpl.mozilla.org/php/getParsedLog.php?id=12462342&tree=Try

Is the TreeMatchContext used for things other than querySelector?
Comment 11 Boris Zbarsky [:bz] 2012-06-07 13:43:42 PDT
Yes, of course. It's used for all selector matching.

The real question is whether the eRelevantLinkUnvisited value is used elsewhere... and if so why ignoring private browsing there makes that elsewhere fail.
Comment 12 :Ehsan Akhgari 2012-06-07 13:51:56 PDT
(In reply to Boris Zbarsky (:bz) from comment #11)
> Yes, of course. It's used for all selector matching.
> 
> The real question is whether the eRelevantLinkUnvisited value is used
> elsewhere... and if so why ignoring private browsing there makes that
> elsewhere fail.

Some of the TreeMatchContext's in nsStyleSet.cpp are constructed using eRelevantLinkUnvisited, which is why this happens, I think.

Given that, can we really avoid accessing the docshell here?
Comment 13 Boris Zbarsky [:bz] 2012-06-07 14:08:56 PDT
For the querySelector case, absolutely.  We only need to touch the docshell to find out whether it's OK to tell the consumer about visitedness stuff, but querySelector doesn't expose that to start with...
Comment 14 :Ehsan Akhgari 2012-06-07 14:38:05 PDT
So, is it find if I add a new argument to the TreeMatchContext's ctor which is set by the caller to ask for this check to be skipped?
Comment 15 Boris Zbarsky [:bz] 2012-06-07 14:39:58 PDT
It would be fine by my, esp, seeing as the constructor is inlined so the compiler could optimize this pretty well.
Comment 16 :Ehsan Akhgari 2012-06-07 20:24:50 PDT
Created attachment 631260 [details] [diff] [review]
Patch (v3)
Comment 17 :Ehsan Akhgari 2012-06-07 20:26:13 PDT
http://tbpl.mozilla.org/?tree=Try&rev=a1253bedc70a
Comment 18 Boris Zbarsky [:bz] 2012-06-07 20:52:35 PDT
Comment on attachment 631260 [details] [diff] [review]
Patch (v3)

r=me
Comment 20 :Ehsan Akhgari 2012-06-08 06:24:11 PDT
Comment on attachment 631260 [details] [diff] [review]
Patch (v3)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 722853
User impact if declined: perf regression
Testing completed (on m-c, etc.): locally and test suites being run
Risk to taking this patch (and alternatives if risky): minimal
String or UUID changes made by this patch: none
Comment 21 Ed Morley [:emorley] 2012-06-08 13:55:05 PDT
https://hg.mozilla.org/mozilla-central/rev/e87b81b5796f
Comment 22 Alex Keybl [:akeybl] 2012-06-11 12:15:57 PDT
Comment on attachment 631260 [details] [diff] [review]
Patch (v3)

[Triage Comment]
Low risk perf fix for a FF14 regression. Approved for Aurora 15 and Beta 14.

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