77954 - unnecessary creation of pseudo-elements

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Chris Waterson]

While profiling the test case for bug 56854, I discovered that we appear to be
creating pseudo-elements; specifically, I saw 5.8K calls to
nsHTMLDocument::CreateElement() from
nsCSSFrameConstructor::CreateGeneratedContentFrame(). Given that this page is
pretty vanilla HTML, I was a bit surprised. Logging this as a task to
investigate more deeply. Refer to bug 56854 for more details.

Comment 1

[Chris Waterson]

.

Comment 2

[Chris Waterson]

From bug 56854:

  . 27% of [frame construction] time (504msec, 8% overall) is spent in
    62K calls to nsCSSFrameConstructor::CreateGeneratedContentFrame().
    Although I can't see why this simple HTML page would trigger it,
    we end up with 5.8K calls to nsHTMLDocument::CreateElement(), so it
    looks like we're actually _creating_ content for pseudos here? As far as
    I can tell, the calls to CreateElement() bottom out in
    NS_NewHTMLSpanElement(), so we're ending up with spans for some
    reason. Worth looking into.

I tracked this down to r1.14 of quirk.css, which was checked in to fix bug 5119 
by attinasi. Every <dd>, which the test case in bug 56854 is full of, has a 
:before pseudo that's creating a span. Is there any other way that we could fix 
bug 5119 without using a pseudo? Reassigning bug to hixie.

hixie, attinasi, pierre: please only use :before and :after pseudo elements as a 
last resort to fix layout problems. They are _extremely_ expensive 
performance-wise!

Comment 3

[Chris Waterson]

Attachment: possible fix?

Comment 4

[Chris Waterson]

The above fixes uses these rules for quirky <dd> behavior:

  dd {
    display: inline;
  }
  dd:first-line {
    display: inline;
    margin-left: 40px;
  }
  dl > dd {
    display: block;
    margin-left: 40px;
  }
  dl > dd:first-line {
    margin-left: 0;
  }

which avoids creation of pseudo-content. (It does, unfortunately, expose another 
bug with inline margins.)

Comment 5

[Chris Waterson]

Taking back from hixie. Hixie, comments appreciated!

Comment 6

[Chris Waterson]

This does regress the expanded test case from bug 5119,

  http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19352

Comment 7

[Hixie (not reading bugmail)]

:first-line will ignore the "display: inline" according to CSS2. Also, you
can probably do ":first-line { margin-right: 40px }" with ":first-line { text-
indent: 40px }", which doesn't have the problem with being bidi-incompatible.

Can't we "just" make :before cheaper? I'm intending to use :before in my fix
for <br> (see bug 38370), and we currently use :before for <hr>. Why is it so
expensive? Shouldn't it be just as cheap as another element? Or is this related
to our other weirdness with :before?

A way to make it cheaper without removing the :before would be to change the 
"dd:before" rule to ":not(dl) > dd:before" and then remove the "dl > dd:before"
rule. That way, we only trigger the slowness or invalid pages.

There might be some other optimisations we can make this way...

Comment 8

[Chris Waterson]

> Can't we "just" make :before cheaper? I'm intending to use :before in my fix
> for <br> (see bug 38370), and we currently use :before for <hr>. Why is it so
> expensive? Shouldn't it be just as cheap as another element?

Well, it's not terribly expensive in itself, but in the test case from bug 
56854, it essentially increases the size of the content and frame models by 
about 20% (at least, in terms of node and frame count), since the rules apply to 
every <dd> in the model.

So, the problem with :before and :after is making sure that they apply in very, 
very restricted ways!

Assigning to ian again.

Comment 9

[Chris Waterson]

Also cc'ing mjudge, who checked in the apparently vestigial content node 
creation in nsCSSFrameConstructor::CreateGeneratedContentFrame(). mike: why do 
we need to create a content node here? It's immediately discarded. Looks like 
something to do with ``generated content''?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do we create generated content for 'content: ""', which should be the default?

If we do, that's a bug.  the ':not(dl) > dd' rule would also be a good thing,
but it should only help because it speeds up selector matching.

Comment 11

[Hixie (not reading bugmail)]

David: in nsCSSFrameConstructor around line 1553 we create a <span> element for
generated content which we immediately drop on the floor. Apparently this 
accounts for around 20% of the time on the test in question.

We should file a bug about that if we intend to fix it separately from the
:not() optimisation.

BTW, I wouldn't be at all surprised if we did weird things with content:"".
Last I checked, the style system initialises content to ||, and if it sees
|""| it changes the value of content to be |""|. This should be fixed by 
introducing a -moz-X keyword for the intitial value case (CSS3 will have "auto"
or "normal" or some such).

So... 3 bugs here huh? ;-)

Comment 12

[Chris Waterson]

Filed bug 78055 on the vestigial content node creation.

Comment 13

[Hixie (not reading bugmail)]

Is this bug only about the :not() optimisation now? If so, I could get to it
during one of the coming weekends. Otherwise, I likely won't be getting to this 
before 0.9.2 ships.

Comment 14

[Chris Waterson]

Yes, I think this bug would be sufficiently ``fixed'' using the :not() selector. 
Thanks!

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If :not() fixes it then we should file a separate bug about 'content: ""' (or
perhaps it's the presence of the pseudo-element at all?) causing performance
problems when it should do nothing.

Comment 16

[Chris Waterson]

Well, we have to do _some_ work to figure out that we've got no work to do,
right? (Even if we special-case the empty string.) Anyway, I'm going to
regenerate the profile for bug 56854 today, since several changes have gone in.

(Generally, I think that writing rules that generate an empty :before pseudo
would be considered unsightly given the existence of the :not() pseudo, no?)

Comment 17

[Hixie (not reading bugmail)]

content: "" should do something, according to CSS3 proposals, so I wouldn't get 
to trigger happy about fixing that any time soon. At the moment we seem to 
almost do the right thing per CSS3 proposals of having an initial value of
'auto' or 'none' or whatever, and having the empty string do the old empty 
inline thingy. Or something. This is underspecified. Let's leave it for now.

Please? ;-)

Comment 18

[Chris Waterson]

I just re-profiled bug 56854: fixing bug 78055 chopped off about 2% of the 
time spent in nsCSSFrameConstructor::CreateGeneratedContentFrame(), but it's 
still accounting for about 6% of the time. Hixie, any chance I could get you to 
cough up a quick patch?

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: possible fix using :not

Comment 20

[Chris Waterson]

This helps a bit, but I think I'm realizing that there is something strange with 
the flow of control. We're _still_ getting 62K calls to 
CreateGeneratedContentFrame(), altough now the entirety of the time is spent in 
nsPresContext::ProbePseudoStyleContextFor() (we're not creating empty text 
frames, yay!)

So, I'd say that the above patch resolves this bug, and we can continue the 
probe into the pseudo-style probing (hyuk hyuk! god, I'm like the bad uncle!) in 
bug 77956.

I don't think my r= will carry much weight here, but you've got it if you need 
it.

Comment 21

[Hixie (not reading bugmail)]

r=hixie

Comment 22

[Chris Waterson]

count me as sr=waterson if you want.

Comment 23

[Hixie (not reading bugmail)]

Since David wrote the fix and my CVS access isn't yet approved for the Seamonkey
part of the tree, reassigning to David so he can check in. :-)

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, marking FIXED since Ian thinks 'content: ""' should be different from the
default and bug 77956 exists and I checked the fix in on 2001-05-18 16:09 PDT.

Comment 25

[Chris Petersen]

Marking verified per last comments