stylo: Attr selector matching should perhaps fast-path the empty-string namespace

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: bzbarsky, Assigned: emilio)

Tracking

(Blocks 2 bugs)

53 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

In ServoBindings.cpp, DoMatch does:

  if (aNS) {
    int32_t ns = nsContentUtils::NameSpaceManager()->GetNameSpaceID(aNS,
                                                                    aElement->IsInChromeDocument());

This is the common path; hardly anyone writes attr selectors with a wildcard namespace.

In fact, hardly anyone writes attr selectors with a namespace other than "" (the default).  So in the common case we do the IsInChromeDocument() thing, and the out of line GetNameSpaceID call, which just compares aNS to nsGkAtoms::_empty and returns kNameSpaceId_None without even using the second argument.

We should consider inlining the "is this the null namespace"? check here so we can avoid the extra work of IsInChromeDocument and whatnot.
(In reply to Boris Zbarsky [:bz] from comment #0)
> In fact, hardly anyone writes attr selectors with a namespace other than ""
> (the default).  So in the common case we do the IsInChromeDocument() thing,
> and the out of line GetNameSpaceID call, which just compares aNS to
> nsGkAtoms::_empty and returns kNameSpaceId_None without even using the
> second argument.

Note that we do have stylesheets with @namespace rules that will make some namespaces appear there though...

But I think this is reasonable.
> Note that we do have stylesheets with @namespace rules that will make some namespaces appear there though...

No, we don't.  In particular, the default namespace is not applied to attr selectors.
(In reply to Boris Zbarsky [:bz] from comment #3)
> > Note that we do have stylesheets with @namespace rules that will make some namespaces appear there though...
> 
> No, we don't.  In particular, the default namespace is not applied to attr
> selectors.

Oh, right! Then this should be even better. Feel free to stamp it if you want btw.
Comment on attachment 8888242 [details]
Bug 1382372: Add a fast-path for matching attr selectors with the empty namespace.

https://reviewboard.mozilla.org/r/159192/#review164638

r=me
Attachment #8888242 - Flags: review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6b2a40d10b49
Add a fast-path for matching attr selectors with the empty namespace. r=bz
Comment on attachment 8888242 [details]
Bug 1382372: Add a fast-path for matching attr selectors with the empty namespace.

https://reviewboard.mozilla.org/r/159192/#review165070

(Already landed, but looks fine.)
Attachment #8888242 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/6b2a40d10b49
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee: nobody → emilio
You need to log in before you can comment on or make changes to this bug.