Closed Bug 1449404 Opened 2 years ago Closed 2 years ago

Devirtualize some of nsIContent

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

There're some virtual methods here that don't need to be.
Saves around 15KB of vtables.
Attachment #8963138 - Flags: review?(continuation)
Attachment #8963139 - Flags: review?(continuation)
Attachment #8963140 - Flags: review?(continuation)
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?
Attachment #8963269 - Flags: review?(continuation)
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.