Skip all the QIing stuff in the TreeMatchContext constructor if possible

RESOLVED FIXED in Firefox 14

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: Ehsan)

Tracking

({perf, regression})

unspecified
mozilla16
x86
Mac OS X
perf, regression
Points:
---

Firefox Tracking Flags

(firefox14+ fixed, firefox15+ fixed, firefox16+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

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?
What's the exact semantics of mVisitedHandling?
That's the part I can't recall exactly....

But for the case of querySelector, the semantic is "treat all links as unvisited".
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.?
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.
tracking-firefox14: --- → +
tracking-firefox15: --- → +
tracking-firefox16: --- → +
Created attachment 631078 [details] [diff] [review]
Patch (v1)

Like this?
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #631078 - Flags: review?(bzbarsky)
Comment on attachment 631078 [details] [diff] [review]
Patch (v1)

The if check is backwards, no?
Attachment #631078 - Flags: review?(bzbarsky) → review-
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.
Attachment #631078 - Attachment is obsolete: true
Attachment #631087 - Flags: review?(bzbarsky)
https://tbpl.mozilla.org/?tree=Try&rev=d5514760a513
Comment on attachment 631087 [details] [diff] [review]
Patch (v2)

r=me
Attachment #631087 - Flags: review?(bzbarsky) → review+
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?
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.
(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?
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...
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?
It would be fine by my, esp, seeing as the constructor is inlined so the compiler could optimize this pretty well.
Created attachment 631260 [details] [diff] [review]
Patch (v3)
Attachment #631087 - Attachment is obsolete: true
Attachment #631260 - Flags: review?(bzbarsky)
http://tbpl.mozilla.org/?tree=Try&rev=a1253bedc70a
Comment on attachment 631260 [details] [diff] [review]
Patch (v3)

r=me
Attachment #631260 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e87b81b5796f
Target Milestone: --- → mozilla16
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
Attachment #631260 - Flags: approval-mozilla-beta?
Attachment #631260 - Flags: approval-mozilla-aurora?
status-firefox16: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/e87b81b5796f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
Attachment #631260 - Flags: approval-mozilla-beta?
Attachment #631260 - Flags: approval-mozilla-beta+
Attachment #631260 - Flags: approval-mozilla-aurora?
Attachment #631260 - Flags: approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/9421cb7212c3
http://hg.mozilla.org/releases/mozilla-beta/rev/3a950b9f6fdc
status-firefox14: --- → fixed
status-firefox15: --- → fixed

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.