Closed
Bug 171830
Opened 22 years ago
Closed 22 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•22 years ago
|
Assignee | ||
Comment 1•22 years ago
|
||
Updated•22 years ago
|
Whiteboard: [dev notes]
Assignee | ||
Comment 2•22 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•22 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•22 years ago
|
||
Assignee | ||
Comment 5•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 years ago
|
Flags: blocking1.3b?
Assignee | ||
Comment 17•22 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•22 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•22 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•22 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•22 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•22 years ago
|
Assignee | ||
Comment 22•22 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•22 years ago
|
Attachment #104971 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #113089 -
Flags: superreview?(roc+moz)
Attachment #113089 -
Flags: review?(dbaron)
Assignee | ||
Updated•22 years ago
|
Summary: Don't use nsCSSPropList hints for inline style changes → [FIX]Don't use nsCSSPropList hints for inline style changes
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•22 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•22 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•22 years ago
|
||
Attachment #101209 -
Attachment is obsolete: true
Attachment #113089 -
Attachment is obsolete: true
Comment 28•22 years ago
|
||
*** Bug 194928 has been marked as a duplicate of this bug. ***
Comment 29•22 years ago
|
||
Request changing SEVERITY to CRITICAL because this blocks 195073 which is
SEVERITY = CRITICAL. Also, comment 22 states this patch depends on 117316.
Let me know when you're ready to land.
Assignee | ||
Comment 31•22 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?
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•22 years ago
|
||
This should apply to a current trunk.....
Attachment #115276 -
Attachment is obsolete: true
I already checked it in (based on the merging I did a week or so ago), actually.
Assignee | ||
Comment 35•22 years ago
|
||
Ah, cool. Thank you!
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: 22 years ago
Resolution: --- → FIXED
Comment 37•22 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•22 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•22 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•22 years ago
|
||
*** Bug 156206 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 41•22 years ago
|
||
Note that this caused regression bug 196603
*** 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
•