Closed Bug 762345 Opened 12 years ago Closed 12 years ago

Skip all the QIing stuff in the TreeMatchContext constructor if possible

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16
Tracking Flags:
Tracking Status
firefox14 + fixed
firefox15 + fixed
firefox16 + fixed

People

(Reporter: bzbarsky, Assigned: ehsan.akhgari)

References

Details

(Keywords: perf, regression, Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Patch (v3)
(deleted), patch
bzbarsky
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review
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: --- → +
Attached patch Patch (v1) (obsolete) (deleted) — — Splinter Review
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-
Attached patch Patch (v2) (obsolete) (deleted) — — Splinter Review
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)
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.
Attached patch Patch (v3) (deleted) — — Splinter Review
Attachment #631087 - Attachment is obsolete: true
Attachment #631260 - Flags: review?(bzbarsky)
Comment on attachment 631260 [details] [diff] [review] Patch (v3) r=me
Attachment #631260 - Flags: review?(bzbarsky) → review+
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
Closed: 12 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+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: