Closed Bug 1374017 Opened 7 years ago Closed 6 years ago

stylo: Consider having a "non-wildcard namespace" hashtable in SelectorMap

Categories

(Core :: CSS Parsing and Computation, enhancement, P4)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- ?
firefox63 --- fixed

People

(Reporter: bzbarsky, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1374017: Add namespace bucket for the selector map.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
Servo's SelectorMap has:

    pub id_hash: MaybeCaseInsensitiveHashMap<Atom, Vec<T>>,
    pub class_hash: MaybeCaseInsensitiveHashMap<Atom, Vec<T>>,
    pub local_name_hash: FnvHashMap<LocalName, Vec<T>>,
    pub other: Vec<T>,

whereas Gecko's RuleHash has:

  PLDHashTable mIdTable;
  PLDHashTable mClassTable;
  PLDHashTable mTagTable;
  PLDHashTable mNameSpaceTable;
  RuleValueList mUniversalRules;

So in Gecko, a rule like this:

  [hidden="true"] {
    display: none;
  }

in a stylesheet whose default namespace url is "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" will never get matched against HTML elements at all, because it won't be found in the RuleHash with the element's namespace and won't be in mUniversalRules.  For servo, this rule would end up in "other" and end up getting matched against elements.  If we fix bug 1373800, that just means checking the namespace, but right now we'd doing a getAttribute("hidden") on everything in sight.

For web pages this mostly affects the rules in minimal-xul.css.  So it's not clear whether the extra hashtable lookup is cheaper than matching against those 6 or so wildcard-name selectors at the top of that file... especially if servo doesn't have the namespace in quickly-hashable or already-hashed form.  In Gecko, a namespace is a 32-bit int, so hashing it is pretty fast.

If we did do this, it would also mean that those selectors from minimal-xul.css wouldn't pop up in revalidation selectors for HTML elements.  See bug 1373800 comment 2 for some of the ones that appear there right now.

If we do decide to not do this, we should add a code comment explaining why this is not worth it.
Blocks: 1373362
Priority: -- → P4
I just realized that with bug 1470163 we load mathml.css upfront, which has some pretty nasty selectors that end up in the global bucket for matching and revalidation.

That's not fun, and this should fix it.
Assignee: nobody → emilio
Blocks: 1470163
And to be clear, global revalidation selectors before patch:

 > global revalidation selectors: [RevalidationSelectorAndHashes { selector: Selector(:fullscreen:not(:root):not(:-moz-browser-frame), specificity = 0xc00), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [0, 0, 0] } }, RevalidationSelectorAndHashes { selector: Selector(mroot > :not(:first-child), specificity = 0x401), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [1964724, 1607834, 0] } }, RevalidationSelectorAndHashes { selector: Selector(msub > :not(:first-child), specificity = 0x401), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [1964724, 10408855, 0] } }, RevalidationSelectorAndHashes { selector: Selector(msup > :not(:first-child), specificity = 0x401), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [1964724, 10976181, 0] } }, RevalidationSelectorAndHashes { selector: Selector(msubsup > :not(:first-child), specificity = 0x401), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [1964724, 2936728, 0] } }, RevalidationSelectorAndHashes { selector: Selector(mmultiscripts > :not(:first-child), specificity = 0x401), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [1964724, 4077142, 0] } }, RevalidationSelectorAndHashes { selector: Selector(munder > :not(:first-child), specificity = 0x401), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [1964724, 9473623, 0] } }, RevalidationSelectorAndHashes { selector: Selector(mover > :not(:first-child), specificity = 0x401), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [1964724, 14694342, 0] } }, RevalidationSelectorAndHashes { selector: Selector(munderover > :not(:first-child), specificity = 0x401), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [1964724, 12286474, 0] } }, RevalidationSelectorAndHashes { selector: Selector([xml|space="preserve"], specificity = 0x400), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [0, 0, 0] } }, RevalidationSelectorAndHashes { selector: Selector(details > summary:first-of-type > *|*, specificity = 0x402), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [3062696527, 2628174652, 3582790223] } }, RevalidationSelectorAndHashes { selector: Selector([hidden="true"], specificity = 0x400), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [0, 0, 0] } }, RevalidationSelectorAndHashes { selector: Selector([collapsed="true"], specificity = 0x400), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [0, 0, 0] } }, RevalidationSelectorAndHashes { selector: Selector([moz-collapsed="true"], specificity = 0x400), selector_offset: 0, hashes: AncestorHashes { packed_hashes: [0, 0, 0] } }]


After the patch, only `:fullscreen:not(:root):not(:-moz-browser-frame)` and `details > summary:first-of-type > *|*` remain on the global list.
Comment on attachment 8987305 [details]
Bug 1374017: Add namespace bucket for the selector map.

https://reviewboard.mozilla.org/r/252550/#review259020

::: servo/components/style/selector_map.rs:280
(Diff revision 2)
>          }
>      }
>  }
>  
>  impl<T: SelectorMapEntry> SelectorMap<T> {
>      /// Inserts into the correct hash, trying id, class, and localname.

Mention namespace here.

::: servo/components/style/selector_map.rs:326
(Diff revision 2)
>          };
>  
>          vector.try_push(entry)
>      }
>  
>      /// Looks up entries by id, class, local name, and other (in order).

Mention namespace here.

::: servo/components/style/selector_map.rs:379
(Diff revision 2)
>                      return false;
>                  }
>              }
>          }
>  
> +        if let Some(v) = self.namespace_hash.get(element.namespace()) {

Since you don't add a comment here, and the other ones in this function seem unnecessary, can you remove them?
Attachment #8987305 - Flags: review?(cam) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> I just realized that with bug 1470163 we load mathml.css upfront, which has
> some pretty nasty selectors that end up in the global bucket for matching
> and revalidation.

Good pick up, by the way!
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86726e4cfa46
Add namespace bucket for the selector map. r=heycam
https://hg.mozilla.org/mozilla-central/rev/86726e4cfa46
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: