Closed
Bug 1449404
Opened 6 years ago
Closed 6 years ago
Devirtualize some of nsIContent
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
23.45 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
10.19 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
18.46 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
15.30 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
15.58 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
6.92 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
There're some virtual methods here that don't need to be.
Assignee | ||
Comment 1•6 years ago
|
||
Saves around 15KB of vtables.
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8963138 -
Flags: review?(continuation)
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8963139 -
Flags: review?(continuation)
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #8963140 -
Flags: review?(continuation)
Assignee | ||
Comment 5•6 years ago
|
||
The HasTextForTranslation implementation was just moved, with the nodetype check up front dropped because that's enforced statically now.
Attachment #8963141 -
Flags: review?(continuation)
Assignee | ||
Comment 6•6 years ago
|
||
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 7•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8963139 -
Flags: review?(continuation) → review+
Comment 8•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8963141 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 9•6 years ago
|
||
> 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... ;)
Comment 10•6 years ago
|
||
(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?
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #8963269 -
Flags: review?(continuation)
Comment 12•6 years ago
|
||
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+
Assignee | ||
Comment 13•6 years ago
|
||
> Maybe file a bug about doing a mass replacement of IsNodeOfType, if one doesn't exist already? Filed bug 1449669 and bug 1449670.
Comment 14•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Attachment #8963142 -
Flags: review?(bugs)
Comment 15•6 years ago
|
||
(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.
Updated•6 years ago
|
Attachment #8963142 -
Flags: review?(bugs) → review+
Comment 16•6 years ago
|
||
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
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ad610b422f26 https://hg.mozilla.org/mozilla-central/rev/e7a650eb7ae0 https://hg.mozilla.org/mozilla-central/rev/9e4c2c101487 https://hg.mozilla.org/mozilla-central/rev/a2f5597047da https://hg.mozilla.org/mozilla-central/rev/1d3e400044fc
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•