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

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.

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

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)

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.

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

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.

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

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.

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.

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

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.

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.

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.

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.

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.

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