Closed Bug 696233 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

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.
Depends on: 662489
Attachment #569646 - Flags: review?(dbaron) → review?(Olli.Pettay)
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+
> 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
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.