Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Consider having four separate hashtables with smaller entries in the nth-index cache

RESOLVED FIXED in mozilla10

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

unspecified
mozilla10
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
For querySelector this should give better cache locality (and is a slight performance improvement).  For normal selector matching it might too, esp. in the common case when only one of the four types of selectors applies to a given node.
(Assignee)

Updated

6 years ago
Depends on: 662489
(Assignee)

Comment 1

6 years ago
Created attachment 569646 [details] [diff] [review]
Switch the nth-index cache to having a single hashtable per selector type (from end vs from start and of-type vs not-of-type).  Gives somewhat better cache locality on some workloads and makes entry addition much cheaper because we no longer need to initi
Attachment #569646 - Flags: review?(dbaron)
(Assignee)

Updated

6 years ago
Attachment #569646 - Flags: review?(dbaron) → review?(Olli.Pettay)

Comment 2

6 years ago
Comment on attachment 569646 [details] [diff] [review]
Switch the nth-index cache to having a single hashtable per selector type (from end vs from start and of-type vs not-of-type).  Gives somewhat better cache locality on some workloads and makes entry addition much cheaper because we no longer need to initi

> nsNthIndexCache::IndexDeterminedFromPreviousSibling(nsIContent* aSibling,
>                                                     Element* aChild,
>                                                     bool aIsOfType,
>                                                     bool aIsFromEnd,
>+                                                    const Cache &aCache,
Cache& aCache would be consistent with other parameters.

>+  // This node's index for this cache.
>+  // If -2, needs to be computed.
>+  // If -1, needs to be computed but known not to be 1.
>+  // If 0, the node is not at any index in its parent.
>+  typedef PRInt32 CacheEntry;

huh, this code is...interesting.

>+  /**
>+   * Returns true if aResult has been set to the correct value for aChild and
>+   * no more work needs to be done.  Returns false otherwise.
>+   */
Don't remove the comments you added in the other bug.
Attachment #569646 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 3

6 years ago
> Cache& aCache would be consistent with other parameters.

Yeah, ok.  Fixed.  At some point I should just switch them all...

> huh, this code is...interesting.

dbaron gets the blame for that bit.  :)  Turns out, the "not 1" case is more common (e.g. used in our UA stylesheets).

> Don't remove the comments you added in the other bug.

Indeed.  The patch here predates those comments being added; the actual patch to push has them.

Thank you for the quick review!

Pushed: http://hg.mozilla.org/integration/mozilla-inbound/rev/28afade47ac2
Flags: in-testsuite-
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
Whiteboard: [need review]
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/28afade47ac2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.