Closed
Bug 171830
Opened 21 years ago
Closed 21 years ago
[FIX]Don't use nsCSSPropList hints for inline style changes
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla1.4alpha
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: testcase)
Attachments
(1 file, 7 obsolete files)
I'll attach a patch tomorrow if my floppy drive cooperates and I find my floppies. The basic idea of the patch is as follows: 1) Add AttributeWillChange to nsIDocumentObserver 2) Cache the old style data in nsCSSFrameConstructor::AttributeWillChange 3) Compare the old to the new in nsCSSFrameConstructor::AttributeChanged The current setup passes the cached data out as an nsISupports, then gets it passed back in. I talked to jst, and he does not like that very much.. I'll be attaching the patch with that setup as-is (because it pretty much works) and I'll also attach the conversation he and I had. I'll work on posting some testcases for testing as well.
![]() |
Assignee | |
Updated•21 years ago
|
![]() |
Assignee | |
Comment 1•21 years ago
|
||
Updated•21 years ago
|
Whiteboard: [dev notes]
![]() |
Assignee | |
Comment 2•21 years ago
|
||
This is pretty much complete, imo, pending resolution of jst's concern. There are some testcases to test with (and more are welcome) at: http://web.mit.edu/bzbarsky/www/testcases/dynamic-inline-style-correctness/ http://web.mit.edu/bzbarsky/www/testcases/dynamic-inline-style-performance/
![]() |
Assignee | |
Comment 3•21 years ago
|
||
Comment on attachment 101301 [details] [diff] [review] Work-in-progress Good news -- this seems to fix a few random bugs. Bad news -- this regresses bug 138120. The testcase in that bug triggers the NS_ASSERTION(!parentFrame, "parent frame but no child frame or undisplayed entry"); assertion.... Which seems bad.
![]() |
Assignee | |
Comment 4•21 years ago
|
||
![]() |
Assignee | |
Comment 5•21 years ago
|
||
Comment on attachment 102414 [details] [diff] [review] This fixes the assertion and the regression (apply in addition to, not instead of) Why bother with asserts if they get ignored? I've moved this patch to its proper place -- bug 151883.
Attachment #102414 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 6•21 years ago
|
||
Comment on attachment 101301 [details] [diff] [review] Work-in-progress As suspected, this regresses bug 118415. I'm still not sure whether the patch for that _really_ fixed things in the old world or just papered over the problem... in any case, I need to come up with a fix in the context of the new code.
Attachment #101301 -
Flags: needs-work+
so i think i like jsts suggestion with a servergenerated key better since it less work in the common case (that we're not passing data from WillChange to HasChanged). One way to take care of the memorymanagement is to have the observer hold a hash of nsISupports, that way you should never leak anything. You still might hold on to an object much longer then you need though, if someone aborts, which admittedly is a bad thing.
hmm.. though we could add an AttributeDidntChange notification as well, that is called if someone aborts.
![]() |
Assignee | |
Comment 9•21 years ago
|
||
Hoping to get that regression addressed sometime this week. We need to come to a decision here on the exact way to pass the context around....
Priority: -- → P1
Target Milestone: --- → mozilla1.3alpha
![]() |
Assignee | |
Comment 10•21 years ago
|
||
This resolves all known regressions. Remaining issues: 1) How should the notifications work? 2) What datatype should be used for storing the style data (nsCachedStyleData feels like a hack when all is said and done)... (I'd do a stub impl of nsIStyleContext, but we want to remove nsIStyleContext....) 3) How to avoid code duplication in the CalcDifference functions? 4) Disentangling from some other patches. Point #4 means there's stuff in this patch that's pretty irrelevant to this bug... I'm sorry about that; I'll try to disentangle as soon as I can... in the meantime, some general feedback on the approach and such would be much appreciated.
Attachment #101301 -
Attachment is obsolete: true
Yeah, there's got to be a way to eliminate the duplication of this CalcStyleDifference function. Maybe we could make them both small by using a loop to iterate through the different style structs, instead of writing them all out explicitly?
![]() |
Assignee | |
Comment 12•21 years ago
|
||
Yeah... Another possibility is that we have an nsIStyleContextBase and nsIStyleContext, with the latter inheriting from the former.... nsIStyleContextBase could be what CalcDifference() takes and instead of using nsCachedStyleData to move this info around I could use an nsIStyleContextBase (which would have the minimal number of methods needed to actually function in CalcDifference). Of course this represents even _more_ virtualization of nsIStyleContext, which may not be a good idea.... :(
Can't you just make a 'fake' nsStyleContext with your nsCachedStyleData in it, but decoupled from rules and other style contexts?
![]() |
Assignee | |
Comment 14•21 years ago
|
||
I considered doing that too... I'll give it a shot tonight or tomorrow and see how it affects perf.
![]() |
Assignee | |
Comment 15•21 years ago
|
||
Roc, that works beautifully. ;) This patch has all the irrelevant stuff stripped out and is basically reviewable.... (modulo the specific changes to nsIDocumentObserver) In case people are interested in the perf impact, here're the results on http://web.mit.edu/bzbarsky/www/testcases/dynamic-inline-style-performance/ (3 or 5 trials for each test): WITHOUT PATCH: inlineStyleOnBody: 1224 1205 1224 1208 1225 inlineStyleOnBodyWithSelectorOnStyleAttr: 1440 1378 1379 1441 1379 uselessInlineStyleOnBody: 1601 1606 1626 1627 1626 movingEmptyDivs: 12963 12879 12969 movingFullDivs: 57013 56354 56344 notMovingFullDivs: 46770 46268 46241 WITH PATCH: inlineStyleOnBody: 879 892 875 893 881 inlineStyleOnBodyWithSelectorOnStyleAttr: 1573 1533 1530 1535 1535 uselessInlineStyleOnBody: 431 415 436 419 436 movingEmptyDivs: 14035 13958 13923 movingFullDivs: 51825 51367 51483 notMovingFullDivs: 8693 8344 8217 And testing with http://alladyn.art.pl/gandalf/MozillaTests/dynl.html : WITHOUT PATCH: Creating 200 layers - time - 1174 1153 1195 Moving 200 layers 100 px to left - time - 8326 8627 8446 Cliping 200 layers - time - 310 310 311 Setting Opacity 10% - time - 8938* 8847* 8816* Reverting z-index for 200 layers - time - 307 319 337 Moving with changing opacity and clip - time - 21116* 21031* 21022* Inputing text into 200 layers - time - 512 509 509 Resizing 200 layers - time - 562 587 565 WITH PATCH: Creating 200 layers - time - 1142 1143 1137 Moving 200 layers 100 px to left - time - 8635 8583 8652 Cliping 200 layers - time - 321 318 320 Setting Opacity 10% - time - 24867** 22199** 22133** Reverting z-index for 200 layers - time - 325 297 292 Moving with changing opacity and clip - time - 42236** 40992** 41705** Inputing text into 200 layers - time - 525 517 495 Resizing 200 layers - time - 575 565 567 *No repaints or reflows happen between opacity=100% and opacity=10% due to continuous reframing **Repaints and reflows happen as opacity/position/clip change
Attachment #104642 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 16•21 years ago
|
||
I considered subclassing nsStyleContext for the snapshot (to override PeekStyleData not to look in the rulenode) , but wasn't sure whether making PeekStyleData virtual was ok.... if someone has strong feelings one way or the other, let me know.
Updated•21 years ago
|
Flags: blocking1.3b?
![]() |
Assignee | |
Comment 17•21 years ago
|
||
I'm not getting to this till January, so this is not making 1.3.
Flags: blocking1.3b?
Target Milestone: mozilla1.3alpha → mozilla1.4alpha
Blocks: 157681
Comment 18•21 years ago
|
||
> The current setup passes the cached data out as an nsISupports, then gets it
> passed back in. I talked to jst, and he does not like that very much..
[...]
+/* Strategy for dealing with inline style:
+ *
+ * 1) During AttributeWillChange, see if we already have an inline style
+ * rule. If we do, get the style context and cache the style data and the
+ * pointer (weak ref; we just care about the value) to the style rule.
+ * Pass this data out as aContext.
+ * 2) During AttributeChanged, see if we have a style rule and if so whether
+ * it's the same as the rule we remembered in AttributeWillChange. If the
+ * rules are the same, the style context will not be changing!
+ * 3) If the inline style rules are different, just do a normal re-resolve
+ * and get the hint from that. This will create new style contexts as
+ * needed.
+ */
NS_IMETHODIMP
+nsCSSFrameConstructor::AttributeWillChange(nsIPresContext* aPresContext,
+ nsIContent* aContent,
+ PRInt32 aNameSpaceID,
+ nsIAtom* aAttribute,
+ nsISupports** aContext)
+{
[...]
+ *aContext =
+ NS_STATIC_CAST(nsISupports*,
+ new StyleAttrChangeContext(snapshot, rule, importantRule,
+ aPresContext));
[...]
+}
Possible suggestion/simplication to avoid this issue. What about using the
frame manager instead of dragging the context around? That is, rather than
returning the context as you are doing (just to get it back later on), just
stash it as a property in the frame manager. Then, when AttributeChanged() is
called later on, you can fetch it back from there.
+ //XXXbz We need to decide what the contract is here. Is it OK to call
+ //AttributeWillChange but not AttributeChanged (eg if an error occurs while
+ //changing the attribute)? If so, we should say so explicitly, I think.
It will also avoid this issue. In the case of success (i.e., perfect match of
"willchange" and "changed" pairs), the property is removed. In case of failure
("willchange" without "changed"), there will be at most a dangling entry but it
will overriden if "willchange" is called again, or it will be destroyed when the
manager goes away, leaving no leak behind.
![]() |
Assignee | |
Comment 19•21 years ago
|
||
What about the case when style is being changed on a display:none element (which has a style context in the undisplayed map but has no frame)? I'm not sure I can use the frame manager there (though I _could_ maybe use the undisplayed map itself....)
Comment 20•21 years ago
|
||
> I _could_ maybe use the undisplayed map itself
Seems plausible since the map refers to the old style context data -- until
completion of the maintainance [set->ClearStyleData()] that AttributeChanged() does.
This actually raises one question with your |GetStyleContextFor|... Isn't there
a loophole in your patch in that you are getting the same old context in the
situation of display=none? i.e., are you not testing the same old context
against itself?
![]() |
Assignee | |
Comment 21•21 years ago
|
||
That's the general problem with the inline style changes -- the fact that the style context pointer does _not_ change in the process. That's why I save the old style data, then clear the style context's cache, then compare, forcing the style context to recompute style data in the process... There is an issue I'm looking into with this whole approach (and this may apply to the current code too) where style data may get cleared and this may cause later changes to not trigger the right change hint...
Updated•21 years ago
|
![]() |
Assignee | |
Comment 22•21 years ago
|
||
I'm a _lot_ happier with this one... It still fixes all the problems the old one fixed, but mostly _removes_ code. Perf testing shows that it's within 1-2% of current trunk on all the DHTML tests I've run (except the ones I rigged to show off the benefits of this patch -- we're 10-60% faster than trunk on those ;) ). This patch depends on bug 117316 being fixed so we don't grow the ruletree like mad. Once that happens, the perf here should be retested, but I suspect that it's not going to change much.
![]() |
Assignee | |
Updated•21 years ago
|
Attachment #104971 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•21 years ago
|
Attachment #113089 -
Flags: superreview?(roc+moz)
Attachment #113089 -
Flags: review?(dbaron)
![]() |
Assignee | |
Updated•21 years ago
|
Summary: Don't use nsCSSPropList hints for inline style changes → [FIX]Don't use nsCSSPropList hints for inline style changes
Comment 23•21 years ago
|
||
Comment on attachment 113089 [details] [diff] [review] Alternate (cleaner) approach r=dbaron (although maybe we shouldn't be using the literal 0x7fffffff as the weight without a macro to hide it)
Attachment #113089 -
Flags: review?(dbaron) → review+
Comment 24•21 years ago
|
||
Comment on attachment 113089 [details] [diff] [review] Alternate (cleaner) approach isn't there a PR_INT32_MAX macro for that?
![]() |
Assignee | |
Comment 25•21 years ago
|
||
Yeah, good point. I'll change that to PR_INT32_MAX
Comment on attachment 113089 [details] [diff] [review] Alternate (cleaner) approach yeah baby! I hope that we can hunt down and kill the other users of the hint from the parser...
Attachment #113089 -
Flags: superreview?(roc+moz) → superreview+
![]() |
Assignee | |
Comment 27•21 years ago
|
||
Attachment #101209 -
Attachment is obsolete: true
Attachment #113089 -
Attachment is obsolete: true
Comment 28•21 years ago
|
||
*** Bug 194928 has been marked as a duplicate of this bug. ***
Comment 29•21 years ago
|
||
Request changing SEVERITY to CRITICAL because this blocks 195073 which is SEVERITY = CRITICAL. Also, comment 22 states this patch depends on 117316.
Comment 30•21 years ago
|
||
Let me know when you're ready to land.
![]() |
Assignee | |
Comment 31•21 years ago
|
||
Pretty much any time (as in the patch is ready to go). The problem is finding a time to watch the tree... Can you do Tuesday morning? Should we ask for a carpool?
Comment 32•21 years ago
|
||
I could watch the tree for you if needed (find me on IRC to make sure I'm around), and I don't think there's a need for a carpool.
![]() |
Assignee | |
Comment 33•21 years ago
|
||
This should apply to a current trunk.....
Attachment #115276 -
Attachment is obsolete: true
Comment 34•21 years ago
|
||
I already checked it in (based on the merging I did a week or so ago), actually.
![]() |
Assignee | |
Comment 35•21 years ago
|
||
Ah, cool. Thank you!
Comment 36•21 years ago
|
||
Fix checked in to trunk, 2003-03-06 11:06/07 PST with additional unused variable removal at 13:01 PST.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 37•21 years ago
|
||
+ if (!decl) { + return result; + } + nsCOMPtr<nsICSSLoader> cssLoader; + nsCOMPtr<nsICSSParser> cssParser; + nsCOMPtr<nsIURI> baseURI; + + result = GetCSSParsingEnvironment(mContent, + getter_AddRefs(baseURI), + getter_AddRefs(cssLoader), + getter_AddRefs(cssParser)); + if (NS_FAILED(result)) { + return result; + } Possible leak of |decl|. (2 places). Also missing the check for success in the call of setDecl() and the ruleAbort business in case of failure.
![]() |
Assignee | |
Comment 38•21 years ago
|
||
> Possible leak of |decl|. (2 places). Actually, no. |decl| is not addrefed by GetCSSDeclaration. > and the ruleAbort business in case of failure Nom the only case in which RuleAbort() is needed is if the decl was never set in a rule (and the decl we get out of GetCSSDeclaration is guaranteed to be set in a rule). RuleAbort() is a fancy name for "delete" in the case of CSSDeclarations... and a decl owned by a rule will be released by the rule when the rule dies. Now we _could_ end up with a double-delete of the decl if SetHTMLAttribute fails in SetCSSDeclaration when called from GetCSSDeclaration... Bug 196271 filed. Also filed bug 196273 on general nsCSSDeclaration memory management.
![]() |
Assignee | |
Comment 39•21 years ago
|
||
Hmm... the not clearing of display data seems to have fixed some {ib} bugs (eg bug 175686). It'd be good to retest the other {ib} bugs.
![]() |
Assignee | |
Comment 40•21 years ago
|
||
*** Bug 156206 has been marked as a duplicate of this bug. ***
![]() |
Assignee | |
Comment 41•21 years ago
|
||
Note that this caused regression bug 196603
Comment 42•20 years ago
|
||
*** Bug 81678 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•