Closed Bug 1382372 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

53 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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.
Blocks: stylo-perf
(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: 3 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.