Closed
Bug 78695
Opened 24 years ago
Closed 23 years ago
[CSS] Rule matching performance improvements
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla0.9.2
People
(Reporter: hyatt, Assigned: hyatt)
References
Details
(7 keywords, Whiteboard: [Hixie-P1])
Attachments
(14 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•24 years ago
|
||
Assignee | ||
Comment 3•24 years ago
|
||
Updated•24 years ago
|
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.1
Assignee | ||
Comment 4•24 years ago
|
||
Geez, Hixie, do you think you added enough keywords? ;)
Comment 5•24 years ago
|
||
oops, forgot a couple.
By the way, no specific test cases are needed. I will be testing the changes using
existing tests, and any new ones I think would be useful, once a branch is cut.
Assignee | ||
Comment 6•24 years ago
|
||
BTW, this patch looks much bigger than it really is, since my changes and
pierre's clash. I haven't yet attempted a merge.
Assignee | ||
Comment 7•24 years ago
|
||
Oh, and don't bother trying to apply it. It doesn't work. :)
Assignee | ||
Comment 8•24 years ago
|
||
Assignee | ||
Comment 9•24 years ago
|
||
Assignee | ||
Comment 10•24 years ago
|
||
Assignee | ||
Comment 11•24 years ago
|
||
The 5/09 patch should work completely. There are no known issues. 7 structs
out of 15 have been converted (font, border, margin, padding, outline, xul,
list). 8 remain (text, color, position, display, ui, content, print, table).
In addition, I still have to do the rewrite of the HTML attribute mapping code
to achieve rule sharing (in addition to computed style data sharing). This
could conceivably be staged in after a landing however. All depends on how
picky people get about memory use.
David, if you're going to rewrite/touch HTML style attributes could you please
take a look at bug 68061 and see if you can kill that in the process. Thanks!
(assuming that you're talking about all style (bgcolor,nowrap,etc.) attrs, not
just the attr style)
Assignee | ||
Updated•24 years ago
|
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Assignee | ||
Updated•24 years ago
|
Summary: Rule matching performance improvements → [CSS] Rule matching performance improvements
Assignee | ||
Comment 13•24 years ago
|
||
I won't be posting any more patches to this bug until I'm done with the
conversion. Those of you who wish to test the branch or follow the progress
can pull the following branch:
Style_20010509_Branch
I have now converted 8 out of 15 structs (position was just converted and
landed on the branch).
Comment 14•24 years ago
|
||
rbs: heads up -- this is going to require changes to MathML
Assignee | ||
Comment 15•24 years ago
|
||
Progress report: I've updated selector matching on the branch to use jst's new
IsContentOfType to avoid the excessive QI in SelectorMatchesData construction.
I've also made style contexts and selector matching data come out of the pres
shell arena (along with the rule nodes and style data structs).
On the current branch, I've removed all of the old-style style sharing, and
those structs that haven't been converted to the new system are being propagated
down the whole style tree still. This means you'll see slightly higher memory
numbers that should plummet nicely as I convert the remaining structs.
Comment 16•23 years ago
|
||
Are you going to build upon style context sharing or style data sharing?
Style data sharing had more potential for savings because of its low granularity
(it is more likely to have a match between two style data structs, than to have
a match between two large style contexts). The full potential wasn't totally
exploited in pierre's initial implementation. He allowed sharing only in the
line of descendants, i.e., sharing was only possible between a child and its
ancestors. If there are identical style data structs in disjointt subtrees,
there would be some duplication (see also bug 79334). Despite this apparent
limitation, he observed a substantial savings (about 60% reduction). The full
potential could have been achieved by abstracting the style data storage (e.g.,
pierre alluded to a hash). I would have thought that style data sharing would
have been the way to go in combination with your new rule resolver system (maybe
after changing that 'blob' thing back to its previous name!) Or I am off target
and it is no more relevant? I am under the impression at the end of the day,
these data structs need to be stored somewhere, and as consequence the issue of
how to effectively and efficiently share them is still around.
Comment 17•23 years ago
|
||
s/I am under the impression/I am under the impression that/
Comment 18•23 years ago
|
||
As time goes by, it gets less and less convenient to find things on the
newsgroups than in bugzilla. So for the "StyleData Libray" idea, I am dumping
details from the "Style Sharing on Steroids" thread (an interesting reading,
BTW).
http://groups.google.com/groups?hl=en&lr=&safe=off&ic=1&th=7a72616ebc070114,1
http://groups.google.com/groups?hl=en&lr=&safe=off&ic=1&th=326d4329b0baddd5,5
<pierre-speaking>
Small implementation details (before I forget them):
- We should have as many hash tables as we have types of style structures
(currently 14 or 15). The hash tables would be indexed by the CRC, and owned
by the PresContext.
- The hash tables would store the style strucs inside a wrapper class that
stores the CRC and the refCount.
- The nsStyleContextData would store hash keys instead of pointers (except
for these nsStyleContextData that are affected by the hack for
39618/gfxScrollFrames).]
</pierre-speaking>
[Also, an implementation of this set of multiple hashtables would need to start
on solid footings -- pldhash, as waterson described in bug 73653]
Assignee | ||
Comment 19•23 years ago
|
||
Another progress report. Tonight I converted the table struct and performed
some more optimizations geared at tables.
Some interesting performance tidbits:
(a) I'm now down to 830 ms on jrgm's page load tests.
(b) The 100x100 color table stress test has dropped from 18 seconds on the
trunk to 4.5 seconds on my branch.
Assignee | ||
Comment 20•23 years ago
|
||
rbs, I achieve sharing of structs naturally in the new system. There's no need
to keep any of the old sharing code around.
Assignee | ||
Comment 21•23 years ago
|
||
My algorithm works like this. There are two trees, a style context tree and a
rule tree. You already know what the style context tree is (it maps roughly to
the DOM tree). The rule tree is a tree of rule nodes, where each node contains
a rule.
A branch of the rule tree from root to some destination node represents a set
of rules that are matched for a given style context. Style contexts, rather
than storing an array of rules, now just store a single pointer to a rule node
in the rule tree. By walking from that node up to the root, the context can
see all the rules that it matched.
Nodes in both trees can store style structs, which are now divided into two
categories: inherited and reset (based off of default vals of the props in the
structs).
The algorithm for resolving style goes like this:
(1) Check style context for the data. If it has it, return it.
(2) Check style context for an inherit bit for that struct. If the bit is
set, the data is further up the style context tree. Just recur into the parent
style context.
(3) Check rule node. If the rule node has the data, return it.
(4) Check inherit bit for the rule node. If set, then the data is further
up in the rule tree. Just recur into the parent node.
(5) Walk the rule tree from most specific rule back to least specific rule.
At each rule, map the props from that rule into your list. If all properties
get filled in, break out of the loop.
(6) Figure out if you can just inherit in the style tree. If so, set the
inherit bit and just return the parent's data.
(7) Actually compute style data. If after computing the data, you determine
that you have a dependency on your position in the style context tree, cache
the data in the style context tree. Otherwise cache in the rule tree on the
highest possible node (the one that first specified some info for the struct).
This system achieves better sharing than either the style context sharing
scheme or the style data scheme employed by pierre, and it also avoids
recomputing the data when a match is found as well.
Furthermore you can be very smart about dynamic changes to style, e.g., you
don't have to allocate a new style context if when you go into :hover on a node
in the style tree you end up at the same rule node.
Comment 22•23 years ago
|
||
Wow, I now see how it works and where the benefits come from... Looks promising.
For example, the early bail-out at (5) can already buy a lot (as opposed to
enumerating the rules forwards and overwriting the superseded props...).
Another clarification. I guess when you say that each tree can store the
structs, you actually mean that the style contexts have pointers to the structs
that are allocated on demand in your lazy approach, right? Otherwise, even if
the style data structs are not computed, but the (unfilled) structs are
_declared_ in the style contexts, two style contexts that only differ with a few
data would be taking more space than necessary, e.g., (1) and (2) would suggest
that the (unused) child structs would be there even while walking up to the
ancestor. However, if there are only pointers to the style data, then that will
answer my question.
Assignee | ||
Comment 23•23 years ago
|
||
Yes, in fact there are three levels of indirection. I have an
nsCachedStyleData class which can be held in both the style context tree and
the rule node tree.
This class has two pointer members, mInheritedStyleData and mResetStyleData.
mInheritedStyleData has pointer members for all the inherited structs, and
mResetStyleData has pointer members for all the reset structs. It is typically
the case that inherited structs are cached in the style context tree and reset
structs are cached in the rule node tree.
Comment 24•23 years ago
|
||
I am satisfied with these clarifications (and I guess other knowledgeable folks
interested in nifty algorithms and data structures have benefited from the
details too...) These details show that the system is built on intelligible
grounds and seems highly equiped to be a hit. Looking forward to see it in
action.
Comment 25•23 years ago
|
||
For information, there was a typo in "Additional Comments From
rbs@maths.uq.edu.au 2001-05-11 19:31":
s/bug 73653/bug 73553/
Assignee | ||
Comment 26•23 years ago
|
||
I converted color and background structs over today and implemented a new arena-
allocated hash key for the transition tables of the rule tree. I am now down
to 815 ms on jrgm's page load tests.
11 structs have been converted. 4 remain. (Content, UI, display, and text.)
Comment 27•23 years ago
|
||
My 2 cents (er... my 300 VN Dongs). I haven't read in detail but the 2 flags
inherit/reset may not be sufficient. We also have "inherit computed value" for
text size and possibly speech volume (does it exists?).
Comment 28•23 years ago
|
||
Inheritance always operates on computed values (except for scaling factor
line-heights, but you could consider the scaling factor itself the computed
value there).
Assignee | ||
Comment 29•23 years ago
|
||
13 out of 15 structs have now been converted. The only two that remain are
text and user interface.
Hixie, if you could test the various properties of the content struct, that
would probably be wise... the page load tests don't really use any of those
properties, so it's hard to know if I got the conversion right.
The properties to test are:
content
quotes
counter-increment
counter-reset
marker-offset
Be sure to test using a value of "inherit" for the counter properties and for
quotes (if we even manage to pass a test case with such a beast now).
Comment 30•23 years ago
|
||
I need this to compile on the branch.
Index: nsHTMLReflowState.h
===================================================================
RCS file: /cvsroot/mozilla/layout/base/public/nsHTMLReflowState.h,v
retrieving revision 3.17.24.1
diff -u -r3.17.24.1 nsHTMLReflowState.h
--- nsHTMLReflowState.h 2001/05/16 20:15:06 3.17.24.1
+++ nsHTMLReflowState.h 2001/05/17 02:51:58
@@ -32,6 +32,7 @@
class nsLineLayout;
struct nsStyleDisplay;
+struct nsStyleVisibility;
struct nsStylePosition;
struct nsStyleBorder;
struct nsStyleMargin;
Assignee | ||
Comment 31•23 years ago
|
||
jrgm, fix checked in.
Assignee | ||
Comment 32•23 years ago
|
||
Pulling back in. I'm gonna be ready soon.
Target Milestone: mozilla0.9.2 → mozilla0.9.1
Comment 33•23 years ago
|
||
Hyatt, you need pre-landing builds from your branch for QA by helpers in the
community? Get on chofmann's branch landing plan if possible.
What about BIDI?
/be
Comment 34•23 years ago
|
||
what builds/platforms are availble now that might be posted to
the experimental area?
do we have performance (jrgm/ibench) and memory/leak numbers on
any of the platforms yet? curt/twalker can help genrating the
later if you get them a build...
Lets avoid cramming this in at the 11 hour of 0.9.1 and trying
to sort out the regressions after the tree closes. Better to take
this as an exception after the tree closes, or maybe as the first
0.9.2 check in, and pull to branch if all looks good.
Comment 35•23 years ago
|
||
In nsIStyleContext.h, I noticed you deprecated GetStyleData and
GetMutableStyleData in favor of GetStyle. GetStyle seems bad since it doesn't
force the caller to make the style data struct const. (I just found 2 places in
MathML code where the caller to GetStyle was modifying the struct.) What's the
rationale behind deprecating Get[Mutable]StyleData in favor of GetStyle?
(I just pulled a tree on the branch and fixed MathML / SVG bustage and some
other bustage, but I fixed the MathML / SVG bustage using what I realized are
now deprecated methods on nsIStyleContext.)
Comment 36•23 years ago
|
||
OK... I got a bit confused there. What MathML was doing used to be fine since
GetStyle copied, but now it just gives you a pointer, so the calling code needs
to do the copying itself for whatever it needs a copy of.
I still think GetStyleData would be preferable to GetStyle since it's a lot
easier to modify the style data by accident when using GetStyle. (A type-safe
inline function template (or set of inline functions) to call it would be even
nicer.) Deprecating Get[Mutable|Unique]StyleData certainly makes sense, though.
Comment 37•23 years ago
|
||
>I still think GetStyleData would be preferable to GetStyle since it's a lot
>easier to modify the style data by accident when using GetStyle.
In the old world, GetStyle() was added by Peter Linss at some stage as the
safest and recommended way to ensure that no trampering happens to the style
data. It was a bit slow tough since it copied the data -- but it surely ensured
that the caller wouldn't and couldn't cast away the 'const' pointers. It was a
bridge towards a future work to abstract the internal storage of the data, I
think.
It can be deprecated/removed in the new world if necessary. However, it is not
good that it is given the same semantics as GetStyleData() as you pointed out.
Assignee | ||
Comment 38•23 years ago
|
||
David, the deprecated comment is not mine. In fact on the branch I converted
everyone to use GetStyleData and don't plan to deprecate it at all.
Assignee | ||
Comment 39•23 years ago
|
||
I'll remove that deprecated comment, since I didn't put it there in the first
place and it no longer applies.
Assignee | ||
Comment 40•23 years ago
|
||
Here are the known issues with the current branch:
(1) Tables don't reset the font properties or color properties that they're
supposed to. All other quirks for tables should be supported.
(2) DHTML is broken. I have to add mechanism for invalidating branches of the
rule tree when style rules change.
(3) BIDI is broken. I do not like the mExplicitDirection variable that was
added to the style structs. This is a hack along the lines of what was done
for text decorations, and I want this rewritten. I would like to be able to
land the changes without doing this fix. Optimally, those responsible for BIDI
would reengineer this so that the synthetic explicit direction property is not
needed.
(4) Two structs (text and UI) remain. I should have them converted by the end
of the day.
Comment 41•23 years ago
|
||
Are you serious about the 0.9.1 TM? This is a performance benefit, yes, but a
significant stability risk too. I'd have preferred to wait on Pierre's changes
too, and I like these better, but in the end I'm left questioning what the real
contribution is to the 0.9.1 milestone (and and associated beta). That said, I'm
anxious to try it :)
Comment 42•23 years ago
|
||
cc'ing ftang.
ftang, please help us getting your bidi guys take a look at hyatt's new CSS branch
and come up with a better bidi support.
we're expecting a huge performance win with hyatt's new CSS, and we need help with
ironing out bidi issues.
- thanx!
Comment 43•23 years ago
|
||
Assignee | ||
Comment 44•23 years ago
|
||
attinasi, yes, I'm not going to try to get it into 0.9.1 unless we establish
that it's stable enough to go in. Current thinking is that this should land at
the start of 0.9.2. I've put the bug in 0.9.1 to reflect the fact that I'm
working on it right now and that people can start testing it now. I'm fine
with waiting until 0.9.2 though if it turns out that there are too many
regressions or issues with the patch.
Comment 45•23 years ago
|
||
To add to the list of problems with this branch: last I checked, it didn't
compile with DEBUG turned on. dbaron: Does your patch solve this, or does it
just solve MathML and SVG problems for opt builds? (BTW, thanks for doing that!)
Comment 46•23 years ago
|
||
Yes, my patch fixes the DEBUG build problems too (the last 2 changes).
Assignee | ||
Comment 47•23 years ago
|
||
Sure, dbaron, check in.
Assignee | ||
Comment 48•23 years ago
|
||
Well, all 15 structs have been converted, and everything has gone to hell. I
knew text was going to kick my ass. I'm having some sort of problem with
vertical-align in table cells, but I don't understand the visual effect that
I'm seeing.
I'm sure I just made some minor error in conversion somewhere, but I'm too
punchy from lack of sleep to find it. :(
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Assignee | ||
Comment 49•23 years ago
|
||
Ok, the branch is happy again and ready for testing. We're back now to the
three known issues (DHTML using .style.blah broken, BIDI broken, -moz-initial
not implemented).
Assignee | ||
Comment 50•23 years ago
|
||
Notes to self:
(1) Rename GetAttributeMappingFunctions to Function and eliminate second arg.
(2) Eliminate nsIMutableStyleContext and remove from build.
(3) Implement -moz-initial.
(4) Implement RemapStyle to invalidate subtrees in the rule tree and style tree
to repair DHTML, and ensure that it isn't being called too much from layout.
(5) Eliminate MapStyleInto and MapFontStyleInto.
Comment 51•23 years ago
|
||
There were some initial comments about this using less memory and having a
smaller footprint besides the faster perf.
Is this still the case? Any numbers on that?
I assume this should also do good things for the speed of the whole UI itself.
Comment 52•23 years ago
|
||
please contact simon@softel.co.il and mkaply@us.ibm.com
for bidi css related issues.
Assignee | ||
Comment 53•23 years ago
|
||
Current trunk, the table stress test in SeaMonkey takes 38560K.
On my branch, it takes 31200K.
Assignee | ||
Comment 54•23 years ago
|
||
After converting the last two structs, I now get 990 on jrgm's tests.
Something I've done caused a massive slowdown. :( Please don't bother
collecting performance data until I figure out what idiotic thing I did on the
branch. :)
Assignee | ||
Comment 55•23 years ago
|
||
Ok, it was just a fluke. I rebooted and am back down to 820.
Comment 56•23 years ago
|
||
This would help testing on linux:
Index: client.mk
===================================================================
RCS file: /cvsroot/mozilla/client.mk,v
retrieving revision 1.137
diff -u -r1.137 client.mk
--- client.mk 2001/05/07 23:50:39 1.137
+++ client.mk 2001/05/19 08:06:49
@@ -52,14 +52,14 @@
#
# For branches, uncomment the MOZ_CO_TAG line with the proper tag,
# and commit this file on that tag.
-#MOZ_CO_TAG = <tag>
+MOZ_CO_TAG = Style_20010509_Branch
NSPR_CO_TAG = NSPRPUB_CLIENT_BRANCH
PSM_CO_TAG = #We will now build PSM from the tip instead of a branch.
NSS_CO_TAG = NSS_CLIENT_TAG
LDAPCSDK_CO_TAG = LDAPCSDK_40_BRANCH
-ACCESSIBLE_CO_TAG =
-GFX2_CO_TAG =
-IMGLIB2_CO_TAG =
+ACCESSIBLE_CO_TAG = Style_20010509_Branch
+GFX2_CO_TAG = Style_20010509_Branch
+IMGLIB2_CO_TAG = Style_20010509_Branch
BUILD_MODULES = all
#######################################################################
Assignee | ||
Comment 57•23 years ago
|
||
I have merged in with the trunk and re-branched. The new branch tag is:
Style_20010518_Branch
Comment 58•23 years ago
|
||
In my build from yesterday, the following pages look different (sometimes very
subtly different) from the trunk:
http://www.web2010.com/solutions/botw/
http://www.egroups.com/
http://www.libpr0n.com/ (default stylesheet)
All of these involve tables of some sort.
Comment 59•23 years ago
|
||
Am I supposed to be able to build Style_20010518_Branch with --enable-bidi? (It
breaks in nsCaret.cpp for lack of nsStyleDisplay::mDirection.)
Assignee | ||
Comment 60•23 years ago
|
||
I missed checking in a few files. The branch should be ok now. Update in
layout.
Assignee | ||
Comment 61•23 years ago
|
||
Hixie, could you isolate libpr0n down to a test-case and try to determine
what's going wrong? Thanks!
Assignee | ||
Comment 62•23 years ago
|
||
egroups does font size=-1 inside tables, so that probably looks wrong because
i'm not emulating the font cutoff quirk. I suspect the first page is the
same. libpr0n is in strict mode however, so that's the one I'm curious about.
Assignee | ||
Comment 63•23 years ago
|
||
Ok, -moz-initial support has been added to the parser, although only font props
support it right now. Table quirks should be golden now.
I also eliminated nsIMutableStyleContext and GetMutableStyleData from the build.
I did end up with 3 places where I still had to force a copy of the style data,
so I couldn't eliminate this feature completely, although I think getting it
down to 3 is pretty darn good. :)
The only 3 places that have to continue using it are:
(a) The background propagation code that throws backgrounds from the body and
html up to the canvas, etc.
(b) Our pref-based <noframes> hack for an unnamed consumer of Gecko. ;)
(c) The text-align reset that happens on tables inside tags like <center>.
All other uses were eliminated. It should be noted that tables have some
GetMutableStyleData code for rules=" and for setting halign and valign from cols
but that code is not turned on currently anyway (and GetMutableStyleData isn't
necessary to implement those features).
I cannot emphasize enough that the new function, GetUniqueStyleData, should only
be used for hacks, corrections to style data that cannot be achieved any other
way.
A perfect example of when NOT to use this function is to handle collapsing
borders in tables. If you mutate the border data on cells, then inheritance in
CSS will be screwed up, and getComputedStyle will return incorrect answers to
the DOM. This function is evil, and only under the direst of circumstances
should its use be considered.
In nearly all of the places where GetMutableStyleData was used in our tree, it
was completely unnecessary. Once this branch lands, let's try to keep uses of
GetUniqueStyleData to a bare minimum.
Ok, so all that remains now is to rewrite ReResolveStyleContext and to work with
the BiDi guys to get their stuff back up and running.
Assignee | ||
Comment 64•23 years ago
|
||
Some footprint data for your edification. This data was collected using
identical builds from 5/18 (where one has all my changes and the other does
not). These footprint numbers were collected from the Windows Task Manager.
Navigator Window (blank)
TRUNK: 17752K
BRANCH: 17032K
Mail window (nothing selected)
TRUNK: 21768K
BRANCH: 21672K
Now we move on to some Web pages. These numbers were collected using MFCEmbed.
www.yahoo.com
TRUNK: 13592K
BRANCH: 13484K
www.gamespot.com
TRUNK: 15968K
BRANCH: 15468K
100x100 Color Stress Test
TRUNK: 31444K
BRANCH: 23196K
CSS2 Front Page (http://www.w3.org/TR/REC-CSS2/)
TRUNK: 14308K
BRANCH: 14224K
www,ebay.com
TRUNK: 14552K
BRANCH: 14492K
www.voodooextreme.com
TRUNK: 15840K
BRANCH: 15610K
www.mozillazine.org
TRUNK: 13352K
BRANCH: 13208K
So it looks like the branch is a footprint win over the current trunk
in addition to being a performance win. Of particular importance is the
result on very large pages with diverse styles. The table stress test
takes 8 megs less memory than the current trunk!
Assignee | ||
Comment 65•23 years ago
|
||
Here's another good one.
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstruc
tor.cpp
The huge lxr frame construction page.
TRUNK: 36992K
BRANCH: 36584K
Comment 66•23 years ago
|
||
>In nearly all of the places where GetMutableStyleData was used in our tree, it
>was completely unnecessary. Once this branch lands, let's try to keep uses of
>GetUniqueStyleData to a bare minimum.
It is great to see that you have cleaned nearly all of them. This mutable stuff
has caused so much grief and pain to all works to the style system so far that
I wish the "bare minimum" could be zero. May be not right now as you strive to
move on. But sometimes in the not to distant future. So you could flag
GetUniqueStyleData() as deprecated on birth. That will stop people from being
tempted to use it. And afterwards, the existing three places that you left could
possibly be re-mapped to custom made Mozilla style rules that will achieve their
desired effects.
Comment 67•23 years ago
|
||
David,
The old implementation of table collapsing borders was turned off before 6.0 and
the new one (which isn't yet complete) does not manipulate style data.
If you have any insight on fixing bug 915 (column style resolution for
text-align,vertical-align not yet implemented [CASCADE]) and other related
column inheritance problems, that would be great.
Assignee | ||
Comment 68•23 years ago
|
||
karnaze, that's great news. rbs, I think even the last 3 could be eliminated
with enough work. It was just easy for me to keep the hack alive for those
cases.
Assignee | ||
Comment 69•23 years ago
|
||
Performance numbers on jrgm's tests.
Cached performance:
TRUNK: 902 ms
BRANCH: 795 ms
Uncached numbers coming shortly.
Assignee | ||
Comment 70•23 years ago
|
||
Uncached:
TRUNK: 957ms
BRANCH: 858ms
Assignee | ||
Comment 71•23 years ago
|
||
And for those who want a basis of comparison, IE6 beta scores a 411 uncached
and a 272 cached.
Comment 72•23 years ago
|
||
That's like a 12% speedup -- HUGE!
But we need what, 4 more such speedups, to beat the competition?
/be
Comment 73•23 years ago
|
||
More than that since we get decreasing returns as the numbers go down. But you
know what? I finally feel like it's within reach. :)
Comment 74•23 years ago
|
||
So it turns out the "bug" with libpr0n.com was actually because hyatt fixed bug
72359 while he was at it ("It just seemed wrong, so I fixed it").
Blocks: 72359
Comment 75•23 years ago
|
||
brendan: if we can ever get -O2 turned on, which presumably depends on the
static linking stuff getting sorted out, that's supposed to be another 9% for
linux...
Assignee | ||
Comment 76•23 years ago
|
||
Because I think this is an uplifting number, I thought I'd share Netscape 4.x's
cached result.
1730.
So we're more than twice as fast as our predecessor. We may be much slower
than IE, but we're kicking 4.x's ass resoundingly now. :)
Comment 77•23 years ago
|
||
Just wanted to say that sometimes the new style builds result in a lot more
memory usage. I have a simple page with the stats:
Mozilla 2001052023-Style, virtual 10,428K; mem 18,104K
Mozilla 2001052104 , virtual 10,396K; mem 18,008K
Now if this page is made more complex(its a web application) by displaying say
1500 tabular rows of detail all styled:
Mozilla 2001052023-Style, virtual 24,028K; mem 31,744K
Mozilla 2001052104 , virtual 22,780K; mem 30,428K
I can attach the first page if needed, just get with me on #mozillazine (grok).
Assignee | ||
Comment 78•23 years ago
|
||
There is one particular case in the new system that is pathological, and I
suspect it might be what you used.
If you make thousands of elements all with inline style attributes on them, you
will completely defeat sharing in the new system. My opinion, however, is that
this is a pretty pathological thing to do, since typical Web pages either (a)
don't use CSS at all, in which case the deprecated mapped attributes DO provide
good sharing, or (b) you use CSS with classes to avoid duplicating the same
style rules thousands of times.
That said, we could quite easily do the same thing for the style attribute that
is done for HTML mapped attributes, namely ensure that identical style
attributes use the same style rule. I don't feel that this case should hold up
a landing, but we could definitely file a bug on it.
If I'm completely wrong and the page doesn't make extensive use of the style
attribute, then please attach the page to the bug, and I'll look into it.
Comment 79•23 years ago
|
||
I am using the experimental builds for this bug (2001-05-21-10-STYLE nightly) on
Win NT. I have a (so far for me) 100% reproducible crash on the main BBC news
page:
http://news.bbc.co.uk
The page loads but when I hit reload I get a crash.
Talkback id: TB30772040Z
Comment 80•23 years ago
|
||
David, I think some HTML editors use style attributes a lot - I know ours will
when the CSS-isazation is completed. It might be worth handling that elegantly.
glazman will know for sure, since he is doing the composer work.
Assignee | ||
Comment 81•23 years ago
|
||
Ben, in a current build of the branch I do not crash on the BBC page (or
reloading the page). It works fine for me.
Assignee | ||
Comment 82•23 years ago
|
||
Marc, agreed. I'll split off an 0.9.2 bug on making the style attribute use
the same rule sharing logic that the HTML mapped attributes do.
Assignee | ||
Comment 83•23 years ago
|
||
Grok pointed me to a version of his page, and for me, the branch takes
substantially LESS memory than the trunk. I suspect, grok, that you may not be
using two identical builds (trunk and branch from the same day).
I'm not sure what experimental build people are using, or if it's completely
current.
Note that even the "pathological" style case should beat the trunk nearly all
the time. :)
Assignee | ||
Comment 84•23 years ago
|
||
For anyone attempting to do comparison tests (like footprint), you need to use
a trunk build from May 18th. The current trunk (May 21st) has a substantial
improvement (Gordon's L2 cache) checked in, so you cannot compare this build to
a branch build.
Assignee | ||
Comment 85•23 years ago
|
||
Also, you should be careful about using two builds that share the same
profile. If a page is uncached, the first build you use will cache the page,
and then the second build will pull from the cache. Be careful about this by
either using different profiles or always flushing your cache after launch.
Comment 86•23 years ago
|
||
how about open new window? any gains there?
Assignee | ||
Comment 87•23 years ago
|
||
DHTML is fixed on the branch. Only remaining issue is BiDI, and I've talked to
the IBM folks and have a plan to fix it.
Assignee | ||
Comment 88•23 years ago
|
||
Note that I fixed DHTML in about the most minimal way possible. There are
additional optimizations that could be made in this new system that would
dramatically increase DHTML (and XUL) performance.
I will be filing 5-10 bugs at some point on all the new improvements that can be
made following this bug being fixed. :)
Comment 89•23 years ago
|
||
Comment 90•23 years ago
|
||
Attached a patch for client.mk (unix), client.mak (windows) to pull the style
branch. The patch for unix is tested and works, the patch for windows is
untested, but should work -- i'm just not positive about the PLUGIN_BRANCH and
XPCOM_BRANCH variables in that one.
Comment 91•23 years ago
|
||
i ran the branch, the trunk (5/18), and 4.x on mac and got the following
(cached) on my G4/450
branch: 2045ms
trunk: 2418ms
4.x: 2487ms
so the branch is about 16% faster than the trunk on the same day _and_ the
branch has the window jiggle on every page load, which is slowing it down. The
trunk build doesn't have that jiggle. So the branch time should be even lower.
Comment 92•23 years ago
|
||
> how about open new window? any gains there?
Possibly some small gains (about 50msec on win2k as far as I have measured).
Comment 93•23 years ago
|
||
It looks like DOM access to style is still broken in the lastest testing builds
(2001-05-23-12-Linux). Is that right? I have an HTML tree thingy that expands
and collapses using display: block/none. This works properly in 0.9 and trunk
nightlies but not with this branch. I think that would block landing.
Comment 94•23 years ago
|
||
Jeffrey: URI? DHTML works for me...
Comment 95•23 years ago
|
||
I'm coughing up a testcase right now since the original is over 10000 lines.
Basically what I see is that setting display: none in DOM works, but setting
display: block in DOM has no effect. Here's one of my functions maybe you can
shoot it down ut it works on the trunk:
function expand(e)
{
var item, children;
var i;
// Crawl up the DOM to find the innermost list item
item = e.target;
while (item.nodeType != Node.ELEMENT_NODE)
item = item.parentNode;
while (item.tagName.toLowerCase() != "li")
item = item.parentNode;
// Make every line item below this one visible
children = item.getElementsByTagName("ul");
for (i = 0; i < children.length; i++)
children.item(i).style.display = "block";
return true;
}
Comment 96•23 years ago
|
||
hyatt: I can semi-reliably reproduce the crash we were seeing.
STEPS TO REPRODUCE
1. Go to http://tinderbox.mozilla.org/
2. Follow the links till you get to a Bonsai view of someone's checkins.
(click on the showbuilds.cgi link, then on SeaMonkey, then on someone's
name in the guilty column, then on "Check-ins within 24 hours").
3. Move your cursor out of the window.
3. Hit alt-left and alt-right a lot in rapid succession (i.e., go back and
forwards a lot very quickly, multiple pages in both directions).
STACK TRACE
nsCOMPtr<nsIStyleRule>::nsCOMPtr<nsIStyleRule>() line 533 + 13 bytes
nsRuleNode::WalkRuleTree() line 782
nsRuleNode::GetUIData() line 573 + 34 bytes
nsRuleNode::GetStyleData() line 4394 + 12 bytes
nsStyleContext::GetStyleData() line 402
nsRuleNode::WalkRuleTree() line 856 + 28 bytes
nsRuleNode::GetUIData() line 573 + 34 bytes
nsRuleNode::GetStyleData() line 4394 + 12 bytes
nsStyleContext::GetStyleData() line 402
nsRuleNode::WalkRuleTree() line 856 + 28 bytes
nsRuleNode::GetUIData() line 573 + 34 bytes
nsRuleNode::GetStyleData() line 4394 + 12 bytes
nsStyleContext::GetStyleData() line 402
nsRuleNode::WalkRuleTree() line 856 + 28 bytes
nsRuleNode::GetUIData() line 573 + 34 bytes
nsRuleNode::GetStyleData() line 4394 + 12 bytes
nsStyleContext::GetStyleData() line 402
nsRuleNode::WalkRuleTree() line 856 + 28 bytes
nsRuleNode::GetUIData() line 573 + 34 bytes
nsRuleNode::GetStyleData() line 4394 + 12 bytes
nsStyleContext::GetStyleData() line 402
nsRuleNode::WalkRuleTree() line 856 + 28 bytes
nsRuleNode::GetUIData() line 573 + 34 bytes
nsRuleNode::GetStyleData() line 4394 + 12 bytes
nsStyleContext::GetStyleData() line 402
nsFrame::GetStyleData() line 492 + 21 bytes
nsFrame::GetCursor() line 1785
nsEventStateManager::UpdateCursor() line 1862
nsEventStateManager::PreHandleEvent() line 337
PresShell::HandleEventInternal() line 5506 + 43 bytes
PresShell::HandleEvent() line 5439 + 25 bytes
nsView::HandleEvent() line 377
nsView::HandleEvent() line 350
nsView::HandleEvent() line 350
nsViewManager::DispatchEvent() line 2056
HandleEvent() line 68
nsWindow::DispatchEvent() line 702 + 10 bytes
nsWindow::DispatchWindowEvent() line 723
nsWindow::DispatchMouseEvent() line 4051 + 21 bytes
ChildWindow::DispatchMouseEvent() line 4298
nsWindow::ProcessMessage() line 2998 + 24 bytes
nsWindow::WindowProc() line 957 + 27 bytes
USER32! 77e148dc()
USER32! 77e14aa7()
USER32! 77e266fd()
nsAppShellService::Run() line 418
main1() line 1093 + 32 bytes
main() line 1391 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
I was using a branch build that I built a few minutes ago. With a build from
yesterday, the same steps were giving me a crash looking for the Outline data,
with only one nsRuleNode::WalkRuleTree call on the stack. (The data didn't look
particularly trashed in that case, either, although I'm guessing that's just
because I was looking at old data in the arena).
Comment 97•23 years ago
|
||
Jeffrey: That code looks legitimate, a test case would be much appreciated! :-)
Thanks dude.
Comment 98•23 years ago
|
||
Okay frobbing style from the DOM is definitely broken on this branch. Testcase
http://atari.saturn5.com/~jwb/this.html works on 0.9, works on
2001-05-23-08-trunk, broken on linux style branch build. W3C validated testcase.
Comment 99•23 years ago
|
||
Jeffrey: Great, thanks dude! I can confirm that setting 'display' back
to 'block' on the branch doesn't work.
Comment 100•23 years ago
|
||
hyatt: so, it seems that it's only the properties in the display struct that are
affected. See:
http://www.hixie.ch/tests/adhoc/dom/css/animation/003.xml ('display')
http://www.hixie.ch/tests/adhoc/dom/css/animation/004.xml ('display')
http://www.hixie.ch/tests/adhoc/dom/css/animation/005.xml ('position')
http://www.hixie.ch/tests/adhoc/dom/css/animation/006.xml ('color')
http://www.hixie.ch/tests/adhoc/dom/css/animation/007.xml ('float')
http://www.hixie.ch/tests/adhoc/dom/css/animation/008.xml ('font-weight')
http://www.hixie.ch/tests/adhoc/dom/css/animation/009.xml ('border-style')
...which shows that 'display', 'position' and 'float' are affected, but 'color',
'font-weight' and 'border-style' are not.
Assignee | ||
Comment 101•23 years ago
|
||
I just landed a slew of DHTML fixes. display should behave now. Update in
content and layout.
Assignee | ||
Comment 102•23 years ago
|
||
Hmmm, I fail test 5, hixie. I get into an infinite loop (or rather blockframe
does). This sure looks like buggy block code, but it doesn't happen on my
trunk build. Sigh.
Assignee | ||
Comment 103•23 years ago
|
||
Ok, I now pass all your tests, hixie. All I have to say is, damn, I
underestimated DHTML. :) I had to heavily patch the frame constructor to fix
this stuff, since RecreateFramesForContent has to use the OLD style data when
removing and the NEW style data when inserting (e.g., when you dynamically
change position).
So hopefully DHTML is really put to bed now and I can focus on fixing BiDI.
Assignee | ||
Comment 104•23 years ago
|
||
Hixie, I'm curious if dbaron's global window checkin on May 18th (which I have
on the branch) is causing the crash, since style sheets (and rules) are dying
at really really odd times (only when the GC runs). Update in DOM to pick up
this backout, and see if you can still repro the crash. FWIW, I can't make the
crash happen on my machine at all.
Comment 105•23 years ago
|
||
In fact, mExplicitDirection becomes unnecessary, if we have inheritance bit. I
thought about utilizing such a bit, but that would require more code changes,
than adding new variable to nsStyleDisplay. Then (it was about 1 year ago) we
couldn't realize that bidi would be integrated in mozilla so deeply..
Comment 106•23 years ago
|
||
Hyatt,
At a long shot (you are probably in totally the wrong code) but currently we
only load a single user stylesheet at Mozilla-init time, and we need to be able
to change user stylesheets on the fly using menus/prefs, and have them apply to
the current page. Is this something you can roll in?
It's sort of bug 6782 but not really.
Gerv
Assignee | ||
Comment 107•23 years ago
|
||
Sorry, I'm not going to succumb to feature creep here. :)
Assignee | ||
Comment 108•23 years ago
|
||
I am examining my regression test data now. Only when I feel that I've passed
all the tests (i.e., that the differences I see are ok) will I post my patch.
Stay tuned.
Assignee | ||
Comment 109•23 years ago
|
||
I am passing all the block regression tests finally. As for the table tests,
well, I'm failing 19 of them, so it may be another day or two before I'm
ready. :)
Assignee | ||
Comment 110•23 years ago
|
||
Actually on further inspection, the remaining failed tests are ok. Some of
them are just frame state mismatches even though the geometry is just fine.
The others are caused by my fix to remove the extra padding on CSS table
cells. This causes me to fail a bunch of tests.
Ok, here comes the patch for review and super-review.
Assignee | ||
Comment 111•23 years ago
|
||
Assignee | ||
Comment 112•23 years ago
|
||
Assignee | ||
Comment 113•23 years ago
|
||
Ok, brendan, attinasi, the patch is ready for you to r and sr. The heart of
the new rule code is nsRuleNode.cpp in the content patch. Files that changed
heavily are nsStyleContext.cpp and nsCSSStyleRule.cpp. The style structs were
moved into their own cpp file in content/shared called nsStyleStruct.cpp.
See my comment in this bug for a description of the rule matching algorithm.
Comment 114•23 years ago
|
||
hyatt: -moz-opacity appears to be broken on your branch. See:
http://www.hixie.ch/tests/adhoc/css/mozilla/opacity/007.xml
Note: Ignore the other tests in that directory. They don't all pass on the
trunk either.
Comment 115•23 years ago
|
||
Will the DOM treeWalker support in bug 82625 cause any issues with this bug?
Alan: no, at most it could cause a merge conflict. But most likly not even that
Comment 117•23 years ago
|
||
note: if you want to build the branch, you have to update mozilla/accessibile
mozilla/modules/libpr0n and mozilla/security to the branch tag by hand (either
by editing the top level makefile to include the branch name, or by "cvs up
-r"'ing the specified dirs w/ the branch tag), *after* a full pull from the
top-level, branched, makefile.
Comment 118•23 years ago
|
||
I found one crash in linux, its easy to reproduce:
Goto bugzilla query page and write "perf" to Keywords field and
submit query -> crash.
Here is backtrace:
#0 0x40acfd87 in nsRuleNode::WalkRuleTree ()
#1 0x40acf714 in nsRuleNode::GetOutlineData ()
#2 0x40c051aa in nsRuleNode::GetStyleData ()
#3 0x40b9c54d in nsStyleContext::GetStyleData ()
#4 0x40dbdce5 in nsBlockFrame::Paint ()
#5 0x40dc36f2 in nsContainerFrame::PaintChild ()
#6 0x40dbdfbe in nsBlockFrame::PaintChildren ()
#7 0x40dbde1e in nsBlockFrame::Paint ()
#8 0x40dc36f2 in nsContainerFrame::PaintChild ()
#9 0x40dc35c5 in nsContainerFrame::PaintChildren ()
#10 0x40dd0aa0 in nsHTMLContainerFrame::Paint ()
#11 0x40dd17b1 in CanvasFrame::Paint ()
#12 0x40df54f8 in PresShell::Paint ()
#13 0x40f1d661 in nsView::Paint ()
#14 0x40f26085 in nsViewManager::RenderDisplayListElement ()
#15 0x40f25e47 in nsViewManager::RenderViews ()
#16 0x40f24e3d in nsViewManager::Refresh ()
#17 0x40f2732e in nsViewManager::DispatchEvent ()
#18 0x40f1d1a2 in HandleEvent ()
#19 0x40721076 in nsWidget::DispatchEvent ()
#20 0x40720f96 in nsWidget::DispatchWindowEvent ()
#21 0x407244be in nsWindow::DoPaint ()
#22 0x4072464c in nsWindow::Update ()
#23 0x40724362 in nsWindow::UpdateIdle ()
#24 0x4035da8f in g_idle_dispatch ()
#25 0x4035c987 in g_main_dispatch ()
#26 0x4035d001 in g_main_iterate ()
#27 0x4035d1cc in g_main_run ()
#28 0x40272e57 in gtk_main ()
#29 0x40713446 in nsAppShell::Run ()
#30 0x406f4b5a in nsAppShellService::Run ()
#31 0x0804f877 in main1 ()
#32 0x0805010f in main ()
Comment 119•23 years ago
|
||
(that could just be bug 82359)
Comment 120•23 years ago
|
||
I'm applying the patches now: for me, the test file in the layout patch won't
apply so I removed it. Also, I'm unable to compile - looks like patch is not
dropping the new files in the right spot - ugh. This could take a while ;)
Assignee | ||
Comment 121•23 years ago
|
||
You could also pull the Style_20010518_Branch if you want to do it that way.
The branch is missing a few bug fixes that the patch contains (e.g., bungled
<th> headers), but you could use it to take a look at the heavily modified
files and the new files.
Assignee | ||
Comment 122•23 years ago
|
||
Assignee | ||
Comment 123•23 years ago
|
||
Assignee | ||
Comment 124•23 years ago
|
||
The new patch is updated with the fix for opacity. What changed:
Implemented CheckOutlineProperties.
Added opacity to CheckVisibilityProperties (that's why it didn't work).
Removed my accidental patch to one of the table regression tests.
Merged in with the 5/29 trunk (had to patch SetDefaultBackgroundColor).
Comment 125•23 years ago
|
||
I've been reviewing the diffs all day, and I have a list of comments about
mostly trivial concerns, and a couple of minor design 'issues', but overall I
think that this is an incredible, massive improvement to the style system. It
gets rid of all of the crud that Pierre and I put in to try and make the
original system a little more memory-friendly, and it makes everything much
simpler and (probably) easier to maintain.
I'm digging into the actual rule creation and usage stuff now (once my build
completes) so I'm sure I'll have a lot better understanding of what is going on
then. Also, the attribute mapping changes need some serious fine-tooth-comb
reviewing (or great test cases) to really make sure they are right...
Comment 126•23 years ago
|
||
Writing CSS/DOM test cases is my favorite hobby :) Attinasi, could you please
expand on what sort of activities would tickle potential attribute mapping bugs?
I'll try to whip up some testcases that aren't already in Hixie's house of horrors.
Comment 127•23 years ago
|
||
Comment 128•23 years ago
|
||
Jeffrey: Basically, all the (typically deprecated) HTML attributes. In fact, my
test cases have proved to be of little use with this because most of the errors
hyatt has made were with attributes and not CSS! :-)
Things like:
<table>
<tr align="right">
<th> I should be right aligned </th>
</tr>
</table>
(This is an example the table regression tests caught which I had missed in my
testing of CSS.)
If you want to check specifically CSS stuff, going through the list of all CSS
properties we support (basically the CSS2 properties) and seeing which no longer
make a difference is something we should do too. (For example, '-moz-opacity'
was broken until hyatt fixed that today.)
Assignee | ||
Comment 129•23 years ago
|
||
rbs, to address your comments.
(1) I didn't actually write GetStyle. At the time I was writing this patch, it
was easier to just back pierre's changes out of my tree. That reverted
GetStyle back to the way it was before that patch. IMO GetStyle should be
completely eliminated in favor of GetStyleData anyway, so I'm reluctant to mess
with it now (unless it's to eliminate it). I wasn't interested in patching all
of the GetStyle() call sites to make the change to GetStyleData. I figured
that could wait until after the initial landing.
(2) No, I can't quite get rid of GetUniqueStyleData. As I said in earlier
posts to this bug, there are 3 places where it's still needed.
(3) Rule nodes need the mPresContext. I started out trying to keep it out of
the node, but it just got too messy. It's certainly possible, but it would
involve passing the pres context in to all calls to GetStyleData, which is a
*huge* patch, and again, not one I wanted to undertake in an initial landing.
The folloing testcase involvs bgcolor on tables combined with bgcolor on body in
xhtml:
http://mozilla.org/quality/browser/standards/xhtml/transitional/table_bgcolor_rgb.xml
The testcase dosn't work in trunk either and covered by bug 68061 but it is a
testcase that fails on html-attributes
Comment 131•23 years ago
|
||
If you are looking for test cases, W3 Schools has a stack of
CSS/DHTML/XHTML/whatnot examples. You could visit these and see if they display
properly.
http://www.w3schools.com/default.asp (look on the right in the middle of the page)
Comment 132•23 years ago
|
||
> Additional Comments From David Hyatt 2001-05-30 01:49 -------
>... IMO GetStyle should be
> completely eliminated in favor of GetStyleData anyway, so I'm reluctant to
> mess with it now (unless it's to eliminate it).
I am not comfortable with these public Set/Get Style in their present form. It
looks that something is being sacrificed here for the sake of expediency.
Comment 133•23 years ago
|
||
To prevent Set/Get Style from being misused (or abused) to make changes to the
style data -- which is what can be done with their present form, my preference
would be to:
- make SetStyle private
- revert GetStyle to what it used to be (i.e., its signature was to conveniently
copy to a struct rather than returning a pointer). Or, maybe remove it then at
last resort.
Comment 134•23 years ago
|
||
David, you prefer the GetStyleData call, which is basically the same as the old
GetMutableStyle call, over GetStyle? I am not in agreement. In fact the GetStyle
and SetStyle methods were added to make the old GetMutableStyle call obsolete
and to allow the internal structure of the style context to be abstracted from
the public API, which I believe is the correct direction to head. The problem is
that the internal implementation of the style context is now directly exposed
via the API, and that will make it harder to change. Also, clients can get style
structure and poke values into them, and that cannot be good. Is your preference
strictly based on performance concerns? What would the performance difference be
if we converted all of the calls to GetStyleData to GetStyle?
Assignee | ||
Comment 135•23 years ago
|
||
rbs, copying style structs is inefficient, and it is not a pattern that should
be supported. GetStyleData returns a const pointer, and a caller would have to
explicitly cast that const away in order to modify the data.
Saying that the preferred style accessor method should do a copy over returning
a const pointer just because you're afraid someone will cast away the const and
manipulate the original data is not something I buy. Sorry.
On many Web pages we get style data as much as 200,000 times. We absolutely
should not be copying style data.
Assignee | ||
Comment 136•23 years ago
|
||
Could someone explain what they like about GetStyle vs. GetStyleData? I really
don't care what the signature of the function is or which one we use as long as
a pointer to the original data is returned and NOT a copy of that data.
On small pages, GetStyle & GetStyleData are called tens of thousands of times.
On larger pages (and I don't even mean huge pages here), they're called
hundreds of thousands of times.
One of the reasons I get such a performance improvement is that I don't do any
copying of style data except for the initial computation.
I'm open to any changes that don't involve forcing the accessor function to
create a needless copy of the data. That's just plain ridiculous given how
many times this function (GetStyleData) is called. These structs have strings
(fonts), they have pointer members that have to be malloced
(border/padding/margin/outline/content) on a copy, etc. etc. You do NOT want
to copy these structs just to query for style data values.
Assignee | ||
Comment 137•23 years ago
|
||
Oops, I was thinking of the rule structs for border/padding/margins/outline.
The computed structs don't have pointers, but you still don't want to do a copy
just to access the value.
Comment 138•23 years ago
|
||
The other concern with returning a pointer to the internal data is that of
encapsulation. Even if you are convinced that clients will not poke the data
into the struct, we have done nothing to hide (and make it easier to change) the
way we store the resolved style values. I had discussed with Troy and Eric and
even Pierre another API that would allow the client to pass in structs that had
all of the data they needed rather in one shot, instead of having to get
visibility, display, border and backgroundcolor seperately. And just because you
pass in a struct instead of a pointer to a pointer does not mean that the copy
has to be deep.
As long as wee choose to pass back internal data structures, we limit the
flexibility in the implementation. That is the trade-off of speed vs.
encapsulation I guess, but I think we could get the best of both if we really
think it through. I don't think that this is any worse than what we had before.
So, if we want to fix this it can be done outside of this change I think. My
inclination is to give up on passing style structs outside of the style contexts
at all. Instead, we could invent a more appropriate medium for getting values
out of the contexts, and it ideally would not just require copying of data from
the style structs and it would not limit the implementation of the style
contexts either.
Assignee | ||
Comment 139•23 years ago
|
||
Marc, I agree with you. I would love to eliminate the structs completely, but
it was convenient to keep them around in this initial patch.
As it happens, on the order of 98% of the callers still use GetStyleData
anyway, so GetStyle is almost never used even on the trunk. IMO given that
both functions expose structs and that neither are really ideal, we may as well
err on the side of performance for now.
Comment 140•23 years ago
|
||
With the change making GetStyle take an |nsStyleStruct**| rather than an
|nsStyleStruct&|, shouldn't the change be to a |const nsStyleStruct**| instead?
either that or get rid of it entirely, as rbs suggested. (It also seems like
making SetStyle private would be good, although perhaps not possible. It at
least needs a big "DO NOT USE THIS" comment in the header file.)
Assignee | ||
Comment 141•23 years ago
|
||
I agree. I'll make both those changes.
Comment 142•23 years ago
|
||
I think that these changes are a marked improvement over what we have now.
Provided that we really plan to address some of the important issues that have
been raised (SetStyle visibility, GetStyleData vs GetStyle, presContext in the
ruleNode, for example) outside of this bug then I'd be happy to see this
committed. There is a lot of value to incrrementalism, and I'd really like to
see this land while David still has enough wrist-power left to fix the bugs that
shake out after landing ;)
Assignee | ||
Comment 143•23 years ago
|
||
Assignee | ||
Comment 144•23 years ago
|
||
Assignee | ||
Comment 145•23 years ago
|
||
The new patches change the following:
(1) General cleanup from attinasi's comments. Changed
nsIStyleSet::ClearRuleTree to Shutdown(). Removed nsFormFrame's
DidSetStyleContext. Cleaned up some of the style accessors in layout.
(2) Restored DumpRegressionData to nsStyleContext.cpp and restored the call to
it from layout.
(3) Made GetStyle take a const nsStyleStruct**.
(4) Added an XXXdwh to make SetStyle private.
(5) Added more comments to header files to help clarify some functions (e.g.,
added comments to GetUniqueStyleData and SetStyle in nsIStyleContext.h).
Ok, brendan, sounds like the ball is now in your court. We open for 0.9.2
tomorrow and I can be the first carpool if you give me enough time to make your
suggested changes before then.
Comment 146•23 years ago
|
||
Stepping in for brendan, since he's away this week.
1. |#if 0| the stuff in nsFrameManager.cpp that is /* */ commented out. Marc,
it's your call whether or not you want to land this without style context
regression test data. I'm inclined to say let it land, file a bug to
re-implement.
2. What are the |CheckForFocus()| changes in nsPresShell.cpp?
3. Why addition of |table[align="left"]| stuff in html.css? Is this something
that was previously handled in C++?
4. Found this in your patch to nsFrameFrame.cpp; is this code not compiled?
nsresult rv = NS_OK;
nsCOMPtr<<nsIHTMLContent> content = do_QueryInterface(mContent, &rv);
5. In nsCSSFrameConstructor::CreateGeneratedContentFrame(), I'm not convinced
that it's safe to just nuke the display-type checking code. Perhaps what
we should do here is just fail to create the frame if the display type
is wrong (rather than coercing it, which we do now). But simply removing
it may lead to some crashers...
6. Not sure why we don't need FixUpOuterTableFloat() anymore. Do your changes
to html.css fix that?
7. Again, in ConstructDocElementFrame(), I'm not convinced that we can leave
out the block display type coersion. Couldn't someone supply a document
style sheet that would crash us?
8. ...and in ConstructRootFrame().
9. Might as well remove all of the #ifdef OLD_TABLE_SELECTION stuff in
nsTableCellFrame.cpp
10. Just remove nsTableOuterFrame::AdjustZeroWidth()?
11. Too bad there's all that duplicated code between nsTextFrame.cpp and
nsTextBoxFrame.cpp. Another jihad!
That's all for my commentary in the layout patch. Could you answer to and/or fix
the above things? Do that, and [s]r=waterson on the layout portion.
Assignee | ||
Comment 147•23 years ago
|
||
Chris, addressing your questions.
(1) This is not regression test data. It's something else (the List and SizeOf
stuff). I plan to fix it post landing, but it involves adding List and SizeOf
to nsIRuleNode.
(2) Oops. Ignore that. That's the patch for 83220.
(3) Yes. I was able to remove code from nsHTMLTableElement.cpp and move it
into html.css.
(4) Not sure. I'll have to look at that.
(5)-(7)-(8) This code didn't get removed. It got rewritten and moved. Now
these fixups happen when you compute display data (see ComputeDisplayData in
nsRuleNode.cpp). This also means the fixups only happen once, so that you
avoid doing them every time you create generated frames if you find already
cached data in the rule tree.
(6) Yes.
(9) mjudge wasn't comfortable with me ripping all of that out yet.
(10) Ok.
(11) Yeah, too bad. :)
Comment 148•23 years ago
|
||
Ok, it's official, [s]r=waterson on the layout stuff. I'll look at the content
changes shortly.
Comment 149•23 years ago
|
||
I'm in the process of reviewing the changes to content/html/content/ so unless
someone else wants to do an additional review of that, don't bother reviewing
that part of the content patch.
Component: Style System → ActiveX Wrapper
Target Milestone: mozilla0.9.2 → M1
Updated•23 years ago
|
Component: ActiveX Wrapper → Style System
Target Milestone: M1 → mozilla0.9.2
Comment 150•23 years ago
|
||
I had a close look through the changes to the HTML element classes in content,
and I also looked over most of the other changes in the content diff, but not in
great detail. The only problems I found in the changes to the element code was
the excessive use of local nsCSSValue variables on the stack for no good reason,
and hyatt fixed all those when I pointed it out to him.
I noticed that nsIRuleNode's (and nsIRuleWalker's) GetIID() method is
handwritten, they should be defined with the NS_DEFINE_STATIC_IID_ACCESSOR()
macro (which would fix the missing const in the current implementations).
One additional thing I noticed was that nsIRuleNode.h and nsIRuleWalker.h needs
to be added to content/html/style/public/MANIFEST, right?
Oh, and does nsRuleNode::RuleDetail nsRuleNode::CheckBackgroundProperties(const
nsCSSColor& aColorData) (and friends) really need to be inline? It's a pretty
large method. If this is performance critical, then it fine, and I see it's used
only in one place so sizewise it's not really a problem. Just thought I'd
mention this...
Since I didn't find anything bad to complain about in the diffs, I just hadto
come up with some nits to pick :-), so here goes:
- The indentation in content/html/style/src/makefile.win,
content/html/style/public/Makefile.in, content/shared/src/Makefile.in, and
content/html/style/src/Makefile.in is messed up, spaces vs. tabs.
- The new files should get a new license header with copyright 2001 Netscape in
stead of 1998
- Tabs (nsStyleStruct.h):
+ float mOpacity; // [inherited] percentage
+
+ PRBool IsVisible() const {
+
return (mVisible == NS_STYLE_VISIBILITY_VISIBLE);
+
}
+
+
PRBool IsVisibleOrCollapsed() const {
+
return ((mVisible == NS_STYLE_VISIBILITY_VISIBLE) ||
+
(mVisible == NS_STYLE_VISIBILITY_COLLAPSE));
+
}
+};
Other than that the changes look good to me, I look forward to seeing this on
the trunk.
sr=jst
Comment 151•23 years ago
|
||
it almost makes me cry (with joy) when I see all the great work
going into the construction, testing, and review of this fix..
great job to hyatt and great job to all those that have helped out.
Assignee | ||
Comment 152•23 years ago
|
||
The inlining is deliberate and on Win32 the functions are inlined.
This is IMO a clever optimization technique. The functions you mention are
large but are used exactly once (inside the rule walking loop), so there's no
danger of code bloat. The point of the inlining is to avoid function calls
while in the tight WalkRuleTree loop.
I profiled this loop in an opt build on Win32 and verified that all the
functions I requested be inlined are successfully inlined by the compiler.
Assignee | ||
Comment 153•23 years ago
|
||
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 154•23 years ago
|
||
Leaks went from ~4K to 48K after this patch went in...
Comment 155•23 years ago
|
||
I opened bug 83678 for the leaks (leak-log attached)
Comment 156•23 years ago
|
||
This seems to cause a regression in Bidi - see bug 83958
Comment 157•23 years ago
|
||
Could someone post here the numbers from Viewer (assuming Viewer was updated)
to know exactly how much was saved in terms of memory and performance on local
copies of standard pages? I sent a copy of these pages (Yahoo, Netscape, CNN)
to attinasi and hyatt before I left. The previous numbers are under bug 43457.
For memory footprint, it's not very clear from what I have seen in this bug
report, and the tinderboxes did not change a bit. I regret that nobody objected
that the style context sharing was disabled in the current tree. It would have
been interesting to do a before/after comparison with both optimizations
enabled. Well, I just thought that someone should find something to complain
about. The performance gains, no problem, are tremendous (-12% overall means at
least one third less in the style system!).
Assignee | ||
Comment 158•23 years ago
|
||
The bloat stat on tinderbox is misleading (bordering on useless). It does not
reflect the fact that all of the style system data is now either arena
allocated or stack allocated. In particular, hundreds of thousands of bytes
worth of data is now stack allocated, and though that is still reflected in the
tinderbox bloat statistics, the situation is much improved (since it's not on
the heap any longer).
As for footprint comparisons of pages, we already know that style data sharing
beats style context sharing in terms of footprint. The footprint stats in this
bug demonstrate that the new system beats style data sharing in terms of
footprint (with the most dramatic gain being the 100x100 table stress test,
which saved 8 megs!).
Comment 160•23 years ago
|
||
*** Bug 87193 has been marked as a duplicate of this bug. ***
Comment 161•23 years ago
|
||
*** Bug 77288 has been marked as a duplicate of this bug. ***
Comment 162•23 years ago
|
||
*** Bug 74771 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•