171830 - [FIX]Don't use nsCSSPropList hints for inline style changes

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Conversation with jst

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Work-in-progress

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/

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: This fixes the assertion and the regression (apply in addition to, not instead of)

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

hmm.. though we could add an AttributeDidntChange notification as well, that is
called if someone aborts.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: checkpoint

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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can't you just make a 'fake' nsStyleContext with your nsCachedStyleData in it,
but  decoupled from rules and other style contexts?

Comment 14

[Boris Zbarsky [:bzbarsky]]

I considered doing that too...  I'll give it a shot tonight or tomorrow and see
how it affects perf.  

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Use a temporary style context

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I'm not getting to this till January, so this is not making 1.3.

Comment 18

[rbs]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

[rbs]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Alternate (cleaner) approach

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.

Comment 23

[David Baron :dbaron:]

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)

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 113089 [details] [diff] [review]
Alternate (cleaner) approach

isn't there a PR_INT32_MAX macro for that?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Yeah, good point.  I'll change that to PR_INT32_MAX

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: patch updated to all the stuff that landed earlier today...

Comment 28

[Johnny Stenback (:jst)]

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

Comment 29

[Ronald Tilby]

Request changing SEVERITY to CRITICAL because this blocks 195073 which is
SEVERITY = CRITICAL.  Also, comment 22 states this patch depends on 117316.

Comment 30

[David Baron :dbaron:]

Let me know when you're ready to land.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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

[David Baron :dbaron:]

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.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: merged to trunk again (against the nsIStyleContext removal)

This should apply to a current trunk.....

Comment 34

[David Baron :dbaron:]

I already checked it in (based on the merging I did a week or so ago), actually.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Ah, cool.  Thank you!

Comment 36

[David Baron :dbaron:]

Fix checked in to trunk, 2003-03-06 11:06/07 PST with additional unused variable
removal at 13:01 PST.

Comment 37

[rbs]

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

Comment 38

[Boris Zbarsky [:bzbarsky]]

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

Comment 39

[Boris Zbarsky [:bzbarsky]]

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.

Comment 40

[Boris Zbarsky [:bzbarsky]]

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

Comment 41

[Boris Zbarsky [:bzbarsky]]

Note that this caused regression bug 196603

Comment 42

[David Baron :dbaron:]

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