Closed Bug 78695 Opened 23 years ago Closed 23 years ago

[CSS] Rule matching performance improvements

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.2

People

(Reporter: hyatt, Assigned: hyatt)

References

Details

(7 keywords, Whiteboard: [Hixie-P1])

Attachments

(14 files)

bookkeeping
181.87 KB, patch
Details | Diff | Splinter Review
5/03 evening drop
492.46 KB, patch
Details | Diff | Splinter Review
5/06 Afternoon Drop. Doesn't run. Don't bother.
548.84 KB, patch
Details | Diff | Splinter Review
5/8 4 am drop. This patch works. Known issues: not calcing differences when contexts change so UI feedback can go missing. Haven't rewritten mapping of HTML attributes into margins, so many tables will look odd.
635.10 KB, patch
Details | Diff | Splinter Review
5/09 Patch.
671.15 KB, patch
Details | Diff | Splinter Review
proposed Linux (gcc 2.96) bustage fixes and MathML and SVG bustage fixes
37.23 KB, patch
Details | Diff | Splinter Review
patch client.mk, client.mak to pull style branch
2.00 KB, patch
Details | Diff | Splinter Review
PATCH TO CONTENT FOR REVIEW
933.37 KB, patch
Details | Diff | Splinter Review
PATCH TO LAYOUT FOR REVIEW
287.74 KB, patch
Details | Diff | Splinter Review
NEW FINAL PATCH TO LAYOUT
284.56 KB, patch
Details | Diff | Splinter Review
NEW FINAL PATCH TO CONTENT
934.36 KB, patch
Details | Diff | Splinter Review
Some comments (rbs)
7.12 KB, patch
Details | Diff | Splinter Review
(LAYOUT) Patch that incorporates attinasi/rbs/dbaron comments
286.53 KB, patch
Details | Diff | Splinter Review
(CONTENT) Patch that incorporates attinasi/rbs/dbaron comments
939.47 KB, patch
Details | Diff | Splinter Review
Attached patch bookkeeping — — Splinter Review 

    
    

    
  
Cc:ing some people to bring this newly added bug to their attention.

    
    

    
  


  
Component: Browser-General → Style System
Keywords: css-moz, 
          
            css1, 
          
            css2, 
          
            css3, 
          
            footprint, 
          
            nsCatFood, 
          
            perf
OS: Windows 2000 → All
QA Contact: doronr → ian
Hardware: PC → All
Whiteboard: [Hixie-P1]

    
  
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.1

    
    

    
  
Geez, Hixie, do you think you added enough keywords? ;)


    
    

    
  
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.
Keywords: arch, 
          
            donttest

    
    

    
  
BTW, this patch looks much bigger than it really is, since my changes and 
pierre's clash.  I haven't yet attempted a merge.


    
    

    
  
Oh, and don't bother trying to apply it.  It doesn't work. :)


    
  
Blocks: 78961

    
    

    
  

      
      
      
      

        Attached patch
          
          
        5/09 Patch.
        — 
          — Splinter Review
      

    


  

    
    

    
  
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)

    
  
Target Milestone: mozilla0.9.1 → mozilla0.9.2

    
  
Summary: Rule matching performance improvements → [CSS] Rule matching performance improvements

    
    

    
  
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).


    
    

    
  
rbs: heads up -- this is going to require changes to MathML

    
    

    
  
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.  


    
  
Blocks: 7251, 71668, 72562

    
  
Blocks: 65266

    
    

    
  
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. 

    
    

    
  
s/I am under the impression/I am under the impression that/

    
    

    
  
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]



    
    

    
  
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.



    
    

    
  
rbs, I achieve sharing of structs naturally in the new system.  There's no need 
to keep any of the old sharing code around.


    
    

    
  
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.


    
    

    
  
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.

    
    

    
  
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.



    
    

    
  
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.

    
    

    
  
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/

    
    

    
  
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.)


    
    

    
  
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?).  

    
    

    
  
Inheritance always operates on computed values (except for scaling factor
line-heights, but you could consider the scaling factor itself the computed
value there).

    
  
Depends on: 80887

    
    

    
  
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).


    
    

    
  
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;


    
    

    
  
jrgm, fix checked in.


    
    

    
  
Pulling back in.  I'm gonna be ready soon.

Target Milestone: mozilla0.9.2 → mozilla0.9.1

    
    

    
  
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

    
    

    
  
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.



    
    

    
  
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.)

    
    

    
  
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.

    
    

    
  
>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.

    
    

    
  
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.  

    
    

    
  
I'll remove that deprecated comment, since I didn't put it there in the first 
place and it no longer applies.


    
    

    
  
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.


    
    

    
  
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 :)

    
    

    
  
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!

    
    

    
  
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.


    
    

    
  
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!)

    
    

    
  
Yes, my patch fixes the DEBUG build problems too (the last 2 changes).

    
    

    
  
Sure, dbaron, check in.


    
    

    
  
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. :(


    
  
Target Milestone: mozilla0.9.1 → mozilla0.9.2

    
    

    
  
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).


    
    

    
  
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.

Blocks: 80084

    
    

    
  
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.

    
    

    
  
please contact simon@softel.co.il and mkaply@us.ibm.com
for bidi css related issues. 

    
    

    
  
Current trunk, the table stress test in SeaMonkey takes 38560K.

On my branch, it takes 31200K.


    
    

    
  
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. :)


    
    

    
  
Ok, it was just a fluke.  I rebooted and am back down to 820.


    
    

    
  
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
 
 #######################################################################


    
    

    
  
I have merged in with the trunk and re-branched.  The new branch tag is:

Style_20010518_Branch



    
    

    
  
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.

    
    

    
  
Am I supposed to be able to build Style_20010518_Branch with --enable-bidi?  (It
breaks in nsCaret.cpp for lack of nsStyleDisplay::mDirection.)

    
    

    
  
I missed checking in a few files.  The branch should be ok now.  Update in 
layout.


    
    

    
  
Hixie, could you isolate libpr0n down to a test-case and try to determine 
what's going wrong?  Thanks!


    
    

    
  
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.


    
  
Depends on: 81823

    
    

    
  
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.



    
    

    
  
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!

    
    

    
  
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



    
    

    
  
>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.

    
    

    
  
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.
 

    
    

    
  
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.


    
    

    
  
Performance numbers on jrgm's tests.

Cached performance:
TRUNK:  902 ms
BRANCH: 795 ms

Uncached numbers coming shortly.



    
    

    
  
Uncached:

TRUNK:  957ms
BRANCH: 858ms


    
    

    
  
And for those who want a basis of comparison, IE6 beta scores a 411 uncached 
and a 272 cached.


    
    

    
  
That's like a 12% speedup -- HUGE!

But we need what, 4 more such speedups, to beat the competition?

/be

    
    

    
  
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.  :)

    
    

    
  
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
Blocks: 75559

    
    

    
  
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...

    
    

    
  
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. :)


    
    

    
  
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).

    
    

    
  
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.


    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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.


    
    

    
  
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.  


    
    

    
  
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. :)


    
    

    
  
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.


    
    

    
  
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.


    
    

    
  
how about open new window?  any gains there?

    
    

    
  
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.


    
    

    
  
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. :)


    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
> how about open new window?  any gains there?

Possibly some small gains (about 50msec on win2k as far as I have measured).

    
    

    
  
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.

    
    

    
  
Jeffrey: URI? DHTML works for me...

    
    

    
  
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;
    }


    
    

    
  
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).

    
    

    
  
Jeffrey: That code looks legitimate, a test case would be much appreciated! :-)
Thanks dude.

    
    

    
  
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.

    
    

    
  
Jeffrey: Great, thanks dude! I can confirm that setting 'display' back 
to 'block' on the branch doesn't work.

    
    

    
  
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.

    
    

    
  
I just landed a slew of DHTML fixes.  display should behave now.  Update in 
content and layout.


    
    

    
  
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.


    
    

    
  
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.


    
    

    
  
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.


    
    

    
  
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..


    
    

    
  
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

    
    

    
  
Sorry, I'm not going to succumb to feature creep here. :)


    
    

    
  
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.


    
    

    
  
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. :)



    
    

    
  
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.


    
    

    
  


  

    
    

    
  


  

    
    

    
  
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.


    
  
Blocks: 54542

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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 ()


    
    

    
  
(that could just be bug 82359)

    
    

    
  
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 ;)

    
    

    
  
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.


    
    

    
  


  

    
    

    
  


  

    
    

    
  
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).


    
    

    
  
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...

    
    

    
  
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.

    
    

    
  


  

    
    

    
  
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.)

    
    

    
  
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

    
    

    
  
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)

    
    

    
  
> 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.

    
    

    
  
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.


    
  
Blocks: 49141

    
    

    
  
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?

    
    

    
  
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.


    
    

    
  
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.


    
    

    
  
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.


    
    

    
  
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.

    
    

    
  
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.


    
    

    
  
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.)

    
    

    
  
I agree.  I'll make both those changes.


    
    

    
  
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 ;)

    
    

    
  
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.


    
    

    
  
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.

    
    

    
  
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. :)



    
    

    
  
Ok, it's official, [s]r=waterson on the layout stuff. I'll look at the content
changes shortly.

    
    

    
  
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
Component: ActiveX Wrapper → Style System
Target Milestone: M1 → mozilla0.9.2

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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.


    
  
Blocks: 78766

    
    

    
  
Fixed.

Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
Leaks went from ~4K to 48K after this patch went in... 

    
    

    
  
I opened bug 83678 for the leaks (leak-log attached)

    
    

    
  
This seems to cause a regression in Bidi - see bug 83958

    
    

    
  
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!). 

    
    

    
  
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!).


    
    

    
  
Rubber stamp verification.
Status: RESOLVED → VERIFIED

    
    

    
  
*** Bug 87193 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 77288 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 74771 has been marked as a duplicate of this bug. ***
No longer blocks: 7251

    
  
Blocks: 7251

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: