562688 - Introduce a fast IsElement() API on nsINode and a mozilla::dom::Element class

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Some immediate perf wins, lays the groundwork for more to come, some code cleanup.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Add new IsElement API

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Add mozilla::dom::Element

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Use the new stuff in nsContentList

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Change some more nsIDocument APIs to return Element

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Use the new stuff in named item code

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove remaining eELEMENT consumers in content/base

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove eELEMENT consumers in the rest of content/

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Change broacasters to use Element

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove eELEMENT consumers in dom/

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 442432 [details] [diff] [review]
Remove remaining eELEMENT consumers in content/base

Note that this content/base patch has one substantive change: the one in sNode3Tearoff::AreNodesEqual.  Given that both things are nsIContent, the new code should be equivalent.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove eELEMENT consumers in layout/base

I changed the undisplayed codepath in ReResolveStyleContext because undisplayed->mContent really can't be null.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove eELEMENT consumers in layout/generic

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove eELEMENT consumers in the rest of layout, except layout/style

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove eELEMENt consumers in layout/style, except in the ruleprocessor

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Eliminate eELEMENT consumers in ruleprocessor

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove all remaining eELEMENT consumers

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove the eELEMENT bit

Comment 18

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

Comment on attachment 442437 [details] [diff] [review]
Remove eELEMENT consumers in layout/base

Where you're removing the null-check, could you add an assertion that undisplayed->mContent is not null?

You filed bugs on the two "XXXbz" comments, right?

r=dbaron

Comment 19

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

Comment on attachment 442438 [details] [diff] [review]
Remove eELEMENT consumers in layout/generic

>-static nsIContent*
>+static Element*
> FindElementAncestorForMozSelection(nsIContent* aContent)
> {
>   NS_ENSURE_TRUE(aContent, nsnull);
>   while (aContent && aContent->IsInNativeAnonymousSubtree()) {
>     aContent = aContent->GetBindingParent();
>   }
>   NS_ASSERTION(aContent, "aContent isn't in non-anonymous tree?");
>-  while (aContent && !aContent->IsNodeOfType(nsINode::eELEMENT)) {
>+  while (aContent && !aContent->IsElement()) {
>     aContent = aContent->GetParent();
>   }
>-  return aContent;
>+  return static_cast<Element*>(aContent);

Shouldn't this return statement use AsElement rather than static_cast?

r=dbaron with that


(Though I wonder if it's better practice to do
  using namespace mozilla::dom;
or
  typedef mozilla::dom::Element Element;
.)

Comment 20

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

Comment on attachment 442439 [details] [diff] [review]
Remove eELEMENT consumers in the rest of layout, except layout/style

r=dbaron

Comment 21

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

Comment on attachment 442440 [details] [diff] [review]
Remove eELEMENt consumers in layout/style, except in the ruleprocessor

I think I saw bugs on the 2 XXXbz comments go by in bugmail, so that's good.

Same aside comment about namespaces here.

You should probably change the parameter type of nsSVGUtils::GetFontSize and GetFontXHeight to Element*.

r=dbaron with that

Comment 22

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

Comment on attachment 442441 [details] [diff] [review]
Eliminate eELEMENT consumers in ruleprocessor

Same aside comment about namespaces.

> struct ElementRuleProcessorData : public RuleProcessorData {
>   ElementRuleProcessorData(nsPresContext* aPresContext,
>-                           nsIContent* aContent, 
>+                           mozilla::dom::Element* aElement, 
>                            nsRuleWalker* aRuleWalker)
>-  : RuleProcessorData(aPresContext,aContent,aRuleWalker)
>+  : RuleProcessorData(aPresContext,aElement,aRuleWalker)

While you're changing this line, could you add spaces after the commas?

r=dbaron

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 442429 [details] [diff] [review]
Change some more nsIDocument APIs to return Element

This *will* conflict with peterv's work in bug 560273, but you both probably know that already.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> could you add an assertion that undisplayed->mContent is not null?

Done.

> You filed bugs on the two "XXXbz" comments, right?

Yep, blocked by this bug.

> Shouldn't this return statement use AsElement rather than static_cast?

I had it that way initially; it ends up being:

  return aContent ? aContent->AsElement() : nsnull;

That looks to me like it might have an extra test compared to what I wrote, but maybe it doesn't really matter?

> Though I wonder if it's better practice to do

I'd rather do the using thing, because we'll end up putting more things in that namespace in the future.

> You should probably change the parameter type of nsSVGUtils::GetFontSize and
> GetFontXHeight to Element*.

Done.  This pushed the AsElement calls into nsSVGLength::EmLength and nsSVGLength::ExLength.

> While you're changing this line, could you add spaces after the commas?

Done.

Comment 25

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

(In reply to comment #24)
> > Shouldn't this return statement use AsElement rather than static_cast?
> 
> I had it that way initially; it ends up being:
> 
>   return aContent ? aContent->AsElement() : nsnull;
> 
> That looks to me like it might have an extra test compared to what I wrote, but
> maybe it doesn't really matter?

Oh.  It's fine either way, then, though the above might be clearer simply to show that it might be null...

Comment 26

[Boris Zbarsky [:bzbarsky]]

Pushed:
           http://hg.mozilla.org/mozilla-central/rev/f62d143743a3
           http://hg.mozilla.org/mozilla-central/rev/e0d610569c75
           http://hg.mozilla.org/mozilla-central/rev/bca6860d1d62
           http://hg.mozilla.org/mozilla-central/rev/bc10dcdc3b1e
           http://hg.mozilla.org/mozilla-central/rev/4872aec50aed
           http://hg.mozilla.org/mozilla-central/rev/f5ab6f6df219
           http://hg.mozilla.org/mozilla-central/rev/95d587ae3c40
           http://hg.mozilla.org/mozilla-central/rev/f12ff2c1e50e
           http://hg.mozilla.org/mozilla-central/rev/120ca10f1c08
           http://hg.mozilla.org/mozilla-central/rev/66104f48f155
           http://hg.mozilla.org/mozilla-central/rev/eff11dc0dd22
           http://hg.mozilla.org/mozilla-central/rev/6ac70189d1b8
           http://hg.mozilla.org/mozilla-central/rev/1b7b064ee77a
           http://hg.mozilla.org/mozilla-central/rev/27aa022a527d
           http://hg.mozilla.org/mozilla-central/rev/5dba6e283a1b
           http://hg.mozilla.org/mozilla-central/rev/37cd6605aea2

and tree is _so_ green.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Performance watcher script has this to blame on this checkin:

  Improvement: Dromaeo (CSS) increase 4.83% on MacOSX 10.5.8 Firefox
  Improvement: DHTML decrease 0.25% on XP Firefox
  Improvement: Dromaeo (CSS) increase 3.33% on MacOSX 10.6.2 Firefox

I think there are some other things that it's mis-blaming (e.g. the Linux Dromaeo CSS 2% improvement) that are also to blame on this change.