Closed Bug 562688 Opened 10 years ago Closed 10 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(16 files)

2.96 KB, patch
jst
: review+
sicking
: superreview+
Details | Diff | Splinter Review
6.29 KB, patch
jst
: review+
sicking
: superreview+
Details | Diff | Splinter Review
10.96 KB, patch
jst
: review+
Details | Diff | Splinter Review
131.49 KB, patch
jst
: review+
sicking
: superreview+
Details | Diff | Splinter Review
39.03 KB, patch
jst
: review+
Details | Diff | Splinter Review
24.70 KB, patch
jst
: review+
Details | Diff | Splinter Review
14.89 KB, patch
jst
: review+
Details | Diff | Splinter Review
5.03 KB, patch
jst
: review+
Details | Diff | Splinter Review
8.91 KB, patch
jst
: review+
Details | Diff | Splinter Review
20.59 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
11.34 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
10.41 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
29.40 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
45.83 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
11.16 KB, patch
jst
: review+
Details | Diff | Splinter Review
12.39 KB, patch
jst
: review+
Details | Diff | Splinter Review
Some immediate perf wins, lays the groundwork for more to come, some code cleanup.
Attachment #442426 - Flags: superreview?(jonas)
Attachment #442426 - Flags: review?(jst)
Attachment #442427 - Flags: superreview?(jonas)
Attachment #442427 - Flags: review?(jst)
Attachment #442429 - Flags: superreview?(jonas)
Attachment #442429 - Flags: review?(jst)
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.
I changed the undisplayed codepath in ReResolveStyleContext because undisplayed->mContent really can't be null.
Attachment #442437 - Flags: review?(dbaron)
Attachment #442444 - Flags: review?(jst)
Blocks: 562698
Blocks: 562700
Blocks: 562701
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
Attachment #442437 - Flags: review?(dbaron) → review+
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;
.)
Attachment #442438 - Flags: review?(dbaron) → review+
Comment on attachment 442439 [details] [diff] [review]
Remove eELEMENT consumers in the rest of layout, except layout/style

r=dbaron
Attachment #442439 - Flags: review?(dbaron) → review+
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
Attachment #442440 - Flags: review?(dbaron) → review+
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
Attachment #442441 - Flags: review?(dbaron) → review+
Attachment #442426 - Flags: review?(jst) → review+
Attachment #442427 - Flags: review?(jst) → review+
Attachment #442428 - Flags: review?(jst) → review+
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.
Attachment #442429 - Flags: review?(jst) → review+
Attachment #442430 - Flags: review?(jst) → review+
Attachment #442432 - Flags: review?(jst) → review+
Attachment #442433 - Flags: review?(jst) → review+
Attachment #442434 - Flags: review?(jst) → review+
Attachment #442435 - Flags: review?(jst) → review+
Attachment #442443 - Flags: review?(jst) → review+
Attachment #442444 - Flags: review?(jst) → review+
Attachment #442426 - Flags: superreview?(jonas) → superreview+
Attachment #442427 - Flags: superreview?(jonas) → superreview+
Attachment #442429 - Flags: superreview?(jonas) → superreview+
> 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.
(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...
Keywords: perf
Target Milestone: --- → mozilla1.9.3a5
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.
Duplicate of this bug: 562512
Depends on: 564079
No longer depends on: 564079
Depends on: 614058
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.