Closed Bug 1443553 Opened 6 years ago Closed 6 years ago

Devirtualize a few nsIDocument thingies.

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: emilio, Assigned: emilio)

Details

Attachments

(3 files)

Bug 1443553: Devirtualize nsIDocument::AddObserver / RemoveObserver.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1443553: Devirtualize ContentStateChanged / DocumentStatesChanged / StyleRule*.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1443553: Devirtualize BeginUpdate, FlushPendingNotifications, CreatorParserOrNull.
59 bytes, text/x-review-board-request
smaug
: review+
Details 
      No description provided.
Comment on attachment 8956504 [details]
Bug 1443553: Devirtualize nsIDocument::AddObserver / RemoveObserver.

https://reviewboard.mozilla.org/r/225420/#review231494
Attachment #8956504 - Flags: review?(bugs) → review+
Comment on attachment 8956505 [details]
Bug 1443553: Devirtualize ContentStateChanged / DocumentStatesChanged / StyleRule*.

https://reviewboard.mozilla.org/r/225422/#review231496
Attachment #8956505 - Flags: review?(bugs) → review+
Comment on attachment 8956506 [details]
Bug 1443553: Devirtualize BeginUpdate, FlushPendingNotifications, CreatorParserOrNull.

https://reviewboard.mozilla.org/r/225424/#review231498

Could you re-check that variables are defined only in one class.

::: dom/base/nsDocument.h:1040
(Diff revision 1)
>    // Array of owning references to all children
>    nsAttrAndChildArray mChildren;
>  
>    // Pointer to our parser if we're currently in the process of being
>    // parsed into.
>    nsCOMPtr<nsIParser> mParser;

there is mParser here...

::: dom/base/nsDocument.h:1045
(Diff revision 1)
>    nsCOMPtr<nsIParser> mParser;
>  
>    // Weak reference to our sink for in case we no longer have a parser.  This
>    // will allow us to flush out any pending stuff from the sink even if
>    // EndLoad() has already happened.
>    nsWeakPtr mWeakSink;

So you have mWeakSink here...

::: dom/base/nsIDocument.h:1691
(Diff revision 1)
>  
>    // Observation hooks used to propagate notifications to document observers.
>    // BeginUpdate must be called before any batch of modifications of the
>    // content model or of style data, EndUpdate must be called afterward.
>    // To make this easy and painless, use the mozAutoDocUpdate helper class.
> -  virtual void BeginUpdate(nsUpdateType aUpdateType) = 0;
> +  void BeginUpdate(nsUpdateType aUpdateType);

I guess things go wrong rather soon if one tries to override BeginUpdate in HTMLDocument or somewhere.

::: dom/base/nsIDocument.h:3786
(Diff revision 1)
>    // List of ancestor outerWindowIDs that correspond to the ancestor principals.
>    nsTArray<uint64_t> mAncestorOuterWindowIDs;
>  
> +  // Pointer to our parser if we're currently in the process of being
> +  // parsed into.
> +  nsCOMPtr<nsIParser> mParser;

...and there is mParser here.

::: dom/base/nsIDocument.h:3793
(Diff revision 1)
> +  nsrefcnt mStackRefCnt;
> +
> +  // Weak reference to our sink for in case we no longer have a parser.  This
> +  // will allow us to flush out any pending stuff from the sink even if
> +  // EndLoad() has already happened.
> +  nsWeakPtr mWeakSink;

...and there is mWeakSink here.
Attachment #8956506 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] (unusual timezone [JST] for a week) from comment #6)
> ::: dom/base/nsIDocument.h:1691
> (Diff revision 1)
> >  
> >    // Observation hooks used to propagate notifications to document observers.
> >    // BeginUpdate must be called before any batch of modifications of the
> >    // content model or of style data, EndUpdate must be called afterward.
> >    // To make this easy and painless, use the mozAutoDocUpdate helper class.
> > -  virtual void BeginUpdate(nsUpdateType aUpdateType) = 0;
> > +  void BeginUpdate(nsUpdateType aUpdateType);
> 
> I guess things go wrong rather soon if one tries to override BeginUpdate in
> HTMLDocument or somewhere.

If you want I can revert the BeginUpdate changes.

> ::: dom/base/nsIDocument.h:3786
> (Diff revision 1)
> >    // List of ancestor outerWindowIDs that correspond to the ancestor principals.
> >    nsTArray<uint64_t> mAncestorOuterWindowIDs;
> >  
> > +  // Pointer to our parser if we're currently in the process of being
> > +  // parsed into.
> > +  nsCOMPtr<nsIParser> mParser;
> 
> ...and there is mParser here.
> 
> ::: dom/base/nsIDocument.h:3793
> (Diff revision 1)
> > +  nsrefcnt mStackRefCnt;
> > +
> > +  // Weak reference to our sink for in case we no longer have a parser.  This
> > +  // will allow us to flush out any pending stuff from the sink even if
> > +  // EndLoad() has already happened.
> > +  nsWeakPtr mWeakSink;
> 
> ...and there is mWeakSink here.

Blerg, I had this fixed locally, sorry, apparently mozreview failed to push the changes before.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> > 
> > I guess things go wrong rather soon if one tries to override BeginUpdate in
> > HTMLDocument or somewhere.
> 
> If you want I can revert the BeginUpdate changes.
> 
nah. If someone needs virtual BeginUpdate, that can be added then.

(I have a wip patch somewhere trying to de-virtualize EndUpdate, but since it didn't help with performance after all, I didn't finish it.)
Comment on attachment 8956506 [details]
Bug 1443553: Devirtualize BeginUpdate, FlushPendingNotifications, CreatorParserOrNull.

https://reviewboard.mozilla.org/r/225424/#review231506
Attachment #8956506 - Flags: review?(bugs) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9efb360ef2bc
Devirtualize nsIDocument::AddObserver / RemoveObserver. r=smaug
https://hg.mozilla.org/integration/autoland/rev/0b4aac4a6f1c
Devirtualize ContentStateChanged / DocumentStatesChanged / StyleRule*. r=smaug
https://hg.mozilla.org/integration/autoland/rev/ec884c383255
Devirtualize BeginUpdate, FlushPendingNotifications, CreatorParserOrNull. r=smaug
You need to log in before you can comment on or make changes to this bug.