Devirtualize some of nsIContent

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
Last year
4 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(6 attachments)

There're some virtual methods here that don't need to be.
Saves around 15KB of vtables.
The HasTextForTranslation implementation was just moved, with the nodetype
check up front dropped because that's enforced statically now.
Attachment #8963141 - Flags: review?(continuation)
Please look at this one carefully...  I wanted to devirtualize the things that touch mRefCnt, but I wasn't sure why the refcounting/CC setup was what it was.  If you're not sure either, we should probably talk to Olli.
Attachment #8963142 - Flags: review?(continuation)
Comment on attachment 8963138 [details] [diff] [review]
part 1.  Get rid of nsIContent::SetText

Review of attachment 8963138 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsTextControlFrame.cpp
@@ +1264,5 @@
>    NS_PRECONDITION(!mEditorHasBeenInitialized,
>                    "Do not call this after editor has been initialized");
>  
> +  MOZ_ASSERT_IF(mRootNode->GetFirstChild(),
> +                mRootNode->GetFirstChild()->IsText());

Couldn't this assert be inside the else block without the IF stuff and in terms of childContent? It would be shorter. Also, generally, it is a little unfortunate there's no "MustGetAsText()" method with an assert, though I suppose that adding a virtual method would be counterproductive to the goal of shrinking the vtable...
Attachment #8963138 - Flags: review?(continuation) → review+
Attachment #8963139 - Flags: review?(continuation) → review+
Comment on attachment 8963140 [details] [diff] [review]
part 3.  Get rid of nsIContent::AppendTextTo

Review of attachment 8963140 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/base/nsTextEquivUtils.cpp
@@ +116,5 @@
>  nsresult
>  nsTextEquivUtils::AppendTextEquivFromTextContent(nsIContent *aContent,
>                                                   nsAString *aString)
>  {
> +  if (aContent->IsText()) {

What's the reason for these replacements? Is it just a better way to check the same thing, or is there a semantic change?
Attachment #8963140 - Flags: review?(continuation) → review+
Attachment #8963141 - Flags: review?(continuation) → review+
> Couldn't this assert be inside the else block without the IF stuff and in terms of childContent?

Yes, fixed, but see below.

> Also, generally, it is a little unfortunate there's no "MustGetAsText()" method with an assert

Actually, I can just add one.  I'll post an interdiff in a jiffy.

> What's the reason for these replacements?

Replacing a virtual call by a faster inlined check.  They have identical semantics.  Generally I would like to remove IsNodeOfType...  ;)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> Actually, I can just add one.  I'll post an interdiff in a jiffy.
Sounds good.
 
> > What's the reason for these replacements?
> 
> Replacing a virtual call by a faster inlined check.  They have identical
> semantics.  Generally I would like to remove IsNodeOfType...  ;)

Maybe file a bug about doing a mass replacement of IsNodeOfType, if one doesn't exist already?
Comment on attachment 8963269 [details] [diff] [review]
Interdiff for part 1 adding asserting AsText()

Review of attachment 8963269 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.
Attachment #8963269 - Flags: review?(continuation) → review+
> Maybe file a bug about doing a mass replacement of IsNodeOfType, if one doesn't exist already?

Filed bug 1449669 and bug 1449670.
Comment on attachment 8963142 [details] [diff] [review]
part 5.  Move the cycle collected refcount on content nodes up to nsIContent

Review of attachment 8963142 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, Olli should look at this one. I don't know why nsIContent::OwnedOnlyByTheDOMTree() would return false...
Attachment #8963142 - Flags: review?(continuation)
Attachment #8963142 - Flags: review?(bugs)
(In reply to Andrew McCreight [:mccr8] from comment #14)
> nsIContent::OwnedOnlyByTheDOMTree() would return false...
IIRC the idea was that it is an opt-in optimization. So I didn't have to think the ownership model of some less used nsIContents, like DocumentFragment or so. But that was 6 (really!) years ago.
Attachment #8963142 - Flags: review?(bugs) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad610b422f26
part 1.  Get rid of nsIContent::SetText.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a650eb7ae0
part 2.  Get rid of nsIContent::AppendText.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4c2c101487
part 3.  Get rid of nsIContent::AppendTextTo.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f5597047da
part 4.  Get rid of a few virtual nsIContent methods.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d3e400044fc
part 5.  Move the cycle collected refcount on content nodes up to nsIContent.  r=smaug
Depends on: 1486314
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.