1443553 - Devirtualize a few nsIDocument thingies.

Status: RESOLVED FIXED

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

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1443553: Devirtualize nsIDocument::AddObserver / RemoveObserver.

Review commit: https://reviewboard.mozilla.org/r/225420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225420/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1443553: Devirtualize ContentStateChanged / DocumentStatesChanged / StyleRule*.

Review commit: https://reviewboard.mozilla.org/r/225422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225422/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1443553: Devirtualize BeginUpdate, FlushPendingNotifications, CreatorParserOrNull.

Review commit: https://reviewboard.mozilla.org/r/225424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225424/

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8956504 [details]
Bug 1443553: Devirtualize nsIDocument::AddObserver / RemoveObserver.

https://reviewboard.mozilla.org/r/225420/#review231494

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8956505 [details]
Bug 1443553: Devirtualize ContentStateChanged / DocumentStatesChanged / StyleRule*.

https://reviewboard.mozilla.org/r/225422/#review231496

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8956504 [details]
Bug 1443553: Devirtualize nsIDocument::AddObserver / RemoveObserver.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225420/diff/1-2/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8956505 [details]
Bug 1443553: Devirtualize ContentStateChanged / DocumentStatesChanged / StyleRule*.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225422/diff/1-2/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8956506 [details]
Bug 1443553: Devirtualize BeginUpdate, FlushPendingNotifications, CreatorParserOrNull.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225424/diff/1-2/

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(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 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8956506 [details]
Bug 1443553: Devirtualize BeginUpdate, FlushPendingNotifications, CreatorParserOrNull.

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9efb360ef2bc
https://hg.mozilla.org/mozilla-central/rev/0b4aac4a6f1c
https://hg.mozilla.org/mozilla-central/rev/ec884c383255