Open
Bug 128743
Opened 23 years ago
Updated 2 years ago
Clean up GetRuleCascade so it uses array access rather than enumeration callbacks
Categories
(Core :: CSS Parsing and Computation, defect, P4)
Core
CSS Parsing and Computation
Tracking
()
NEW
Future
People
(Reporter: dbaron, Unassigned)
References
Details
(Keywords: perf, Whiteboard: [whitebox])
Attachments
(1 file, 4 obsolete files)
14.25 KB,
patch
|
bzbarsky
:
review+
waterson
:
superreview+
|
Details | Diff | Splinter Review |
Clean up GetRuleCascade (and the enumeration callbacks it uses) by changing all
the things that use enumerators to use array access. This should make it both
easier to understand and faster.
Reporter | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → Future
Reporter | ||
Comment 1•23 years ago
|
||
To do this, I'd want to add an |UnsafeElementAt| to nsVoidArray that assumes the
element is within the bounds -- i.e., that neither has bounds-checking *nor*
null-checking of mImpl.
Target Milestone: Future → mozilla1.1
Reporter | ||
Comment 2•23 years ago
|
||
Reporter | ||
Comment 3•23 years ago
|
||
This includes some stuff that I noticed while looking into bug 137247.
Attachment #78049 -
Attachment is obsolete: true
Reporter | ||
Comment 4•23 years ago
|
||
I'll future-proof these macros with PR_{BEGIN,END}_MACRO too...
Attachment #79163 -
Attachment is obsolete: true
Reporter | ||
Comment 5•23 years ago
|
||
OK, a summary of what this patch does so far (the bigger changes are at the end):
* removes |EVENT_DEBUG| define that doesn't correspond to any code
* removes heavily bitrotted DEBUG_RULES debugging code (makes no sense with
the rule tree, and I see no need to rewrite it since nobody seems to need it)
* macro-izes my RULE_HASH_STATS stuff so it takes up less room
* Comments HasStateDependentStyle better, and makes a performance optimization
to |IsStateSelector| since it was testing for many selectors for which
HasStatetDependentStyle will never be called. This is the part that is a
followup to bug 5693 (see comment 3).
* Redoes GetRuleCascade a bit by:
+ sorting the final array in the reverse order to allow elimination of
AppendRuleToArray in favor of nsISupportsArray::AppendElements. (This
change is a little confusing: we build the final array by combining a
bunch of order-sorted arrays for each weight. So the change is to sort
the weight arrays in the reverse order and then combine the arrays by a
simple append instead of appending through EnumerateBackwards. Then,
of course, it also changes the enumeration order of this final array.)
+ merges BuildStateEnum and BuildHashEnum
![]() |
||
Comment 6•23 years ago
|
||
- (pseudoClass->mAtom == nsCSSAtoms::dragPseudo) ||
- (pseudoClass->mAtom == nsCSSAtoms::selectionPseudo) ||
I assume that drag and selection are both supposed to fire notifications that
cause these pseudos to be applied as needed, which is why we don't need them
here?
> /**
> * Takes the hashtable of arrays (keyed by weight, in order sort) and
> * puts them all in one big array which has a primary sort by weight
> * and secondary sort by reverse order.
> */
This comment could use some changing, methinks, to indicate that the primary sort
is now by reverse weight and the secondary sort is by order.
That seems fishy, by the way. I would expect the new way to have a sort by
reverse weight and secondary sort by reverse order, so that the first thing in
the list is the thing of highest weight that came latest. I'm guessing the
comment is wrong to start with about the sort orders of the arrays?
The last looks good.
![]() |
||
Comment 7•23 years ago
|
||
Er, the _rest_ looks good. :)
Reporter | ||
Comment 8•23 years ago
|
||
Actually, drag and selection pseudos aren't implemented, and should probably be
completely removed (perhaps as part of bug 3935). :selection isn't even a
pseudo-class -- it's a pseudo-element!
A corrected comment is:
/**
* Takes the hashtable of arrays (keyed by weight, in order sort) and
* puts them all in one big array which has a primary sort by weight
* and secondary sort by order.
*/
(change "reverse order" to "order")
Now that I notice it, I could probably also change AppendRuleToTable to
PrependRuleToTable at some point in the future, and fix the worst-case O(N^2)
(cumulatively) insertion into the rule hash. Maybe I'll do that now...
Reporter | ||
Comment 9•23 years ago
|
||
Actually, no, that would be a bit of a pain unless I convert to pldhash, which
would make this patch even more confusing than it is already. But I'll toss in
an XXX comment.
Reporter | ||
Comment 10•23 years ago
|
||
Attachment #79164 -
Attachment is obsolete: true
![]() |
||
Comment 11•23 years ago
|
||
Comment on attachment 79188 [details] [diff] [review]
more/better comments, in response to bzbarsky
r=bzbarsky
Attachment #79188 -
Flags: review+
Comment 12•23 years ago
|
||
Comment on attachment 79188 [details] [diff] [review]
more/better comments, in response to bzbarsky
sr=waterson, although I'd have left the empty macros...well, empty.
Attachment #79188 -
Flags: superreview+
Reporter | ||
Comment 13•23 years ago
|
||
Attachment 79188 [details] [diff] checked in to trunk, 2002-04-15 15:49 PDT.
Leaving bug open for further work.
Reporter | ||
Comment 14•23 years ago
|
||
This crashes on startup in unrelated code, and I haven't figured out why.
Reporter | ||
Updated•23 years ago
|
Attachment #81286 -
Flags: needs-work+
Reporter | ||
Comment 15•23 years ago
|
||
The above patch should really go on bug 112318.
Comment 16•23 years ago
|
||
cc'ing myself
Reporter | ||
Comment 17•23 years ago
|
||
Comment on attachment 81286 [details] [diff] [review]
failed attempt to convert to pldhash
There is now a working patch on bug 112318 that replaces this one.
Attachment #81286 -
Attachment is obsolete: true
Reporter | ||
Updated•23 years ago
|
Target Milestone: mozilla1.1alpha → Future
Updated•22 years ago
|
Whiteboard: [dev notes]
Updated•22 years ago
|
Whiteboard: [dev notes] → [whitebox]
Reporter | ||
Updated•18 years ago
|
Assignee: dbaron → nobody
Status: ASSIGNED → NEW
QA Contact: ian → style-system
Summary: Clean up GetRuleCascade → Clean up GetRuleCascade so it uses array access rather than enumeration callbacks
Reporter | ||
Updated•13 years ago
|
Priority: P2 → P4
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•