578696 - Stop holding strong refs to mutation observers

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

nsNodeUtils currently holds strong refs to mutation observers when notifying them.  In most cases this isn't needed, and just slows things down.

Patches coming up that make observers responsible for making sure they don't die.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1. Add new macrology

I didn't change the indentation because I plan to make the STRONGREF macro go away in the remaining patches.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2. Make nsXMLEventsManager implement fewer observer methods

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3. No refs when calling ParentChainChanged

Why this is safe, broken down by implementation of ParentChainChanged:

* nsSHEntry: Empty.
* nsRange: Documented to explain why it's safe.
* nsMenuGroupOwnerX: Empty.
* nsDocAccessible: Empty.
* nsXMLEventsManager: Removed.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4. No refs when calling NodeWillBeDestroyed

Why this is safe, broken down by implementation of NodeWillBeDestroyed:

* nsTreeContentView: Holds strong ref with this patch.
* inDOMView: Empty.
* nsLoadCollector: Doesn't drop any strong refs.
* nsSHEntry: Empty.
* nsContentList: Doesn't drop any strong refs.
* nsAttributeTextnode: Doesn't drop any strong refs.
* nsDOMStyleSheetList: Doesn't drop any strong refs.
* nsXULContentBuilder: Holds strong ref with this patch.
* nsXULTemplateBuilder: Already holds strong ref to self.
* nsXULTreeBuilder: Holds strong ref with this patch.
* nsSVGUseElement: Holds strong ref with this patch.
* nsXMLEventsManager: Holds strong ref with this patch.
* nsXMLPrettyPrinter: Already holds a strong ref to itself.
* nsXPathResult: Holds strong ref with this patch.
* txMozillaXSLTProcessor: Holds strong ref with this patch.
* nsMenuGroupOwnerX: Doesn't drop any strong refs.
* nsHTMLAnonymousUtils: Already holds a strong ref to itself.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5. No refs when calling CharacterDataWillChange

Why this is safe, broken down by implementation of CharacterDataWillChange:

* nsSHEntry: Empty.
* nsXMLEventsManager: Removed.
* nsMenuGroupOwnerX: Empty.
* nsDocAccessible: Doesn't drop any strong refs.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 6. CharacterDataChanged

Why this is safe, broken down by implementation of CharacterDataChanged:

* PresShell: Safe under a scriptblocker.
* nsSHEntry: Already holds a strong ref to self.
* nsScriptElement: Safe under a scriptblocker.
* nsRange: Doesn't drop any strong refs.
* nsSVGStyleElement: Safe under a scriptblocker.
* nsSVGUseElement: Doesn't drop any strong refs.
* nsSVGTitleElement: Doesn't drop any strong refs.
* nsTextStateManager: Doesn't drop any strong refs.
* nsXMLEventsManager: Removed.
* nsHTMLTextAreaElement: Holds strong ref with this patch.
* nsAnonDivObserver: Doesn't drop any strong refs.
* nsHTMLStyleElement: Safe under a scriptblocker.
* nsHTMLOutputElement: Doesn't drop any strong refs.
* nsHTMLTitleElement: Doesn't drop any strong refs.
* nsXPathResult: Safe with this patch.
* txMozillaXSLTProcessor: Safe.
* nsMenuGroupOwnerX: Empty.
* nsDocAccessible: Doesn't drop any strong refs.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 7. AttributeWillChange

Why this is safe, broken down by implementation of AttributeWillChange:

* PresShell: Doesn't drop any strong refs.
* nsSHEntry: Empty.
* nsXULDocument: Holds strong ref with this patch.
* nsXMLEventsManager: Removed.
* nsMenuGroupOwnerX: Empty.
* nsDocAccessible: Empty.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 8. AttributeChanged.

Why this is safe, broken down by implementation of AttributeChanged:

* PresShell: Doesn't drop any strong refs.
* nsTreeContentView: Holds strong ref with this patch.
* nsSVGMutationObserver: Doesn't drop any strong refs.
* nsSVGRenderingObserver: Doesn't drop any strong refs.
* inDOMView: Holds strong ref with this patch.
* nsImageMap: Lifetime managed by frame; can't destroy frame.
* nsSHEntry: Already holds a strong ref to self.
* nsScriptElement: Safe under a scriptblocker.
* nsContentList: Safe.
* nsDOMAttribute: Holds strong ref with this patch.
* nsTextNode: Safe under a scriptblocker.
* nsXULContentBuilder: Holds strong ref with this patch.
* nsXULTemplateBuilder: Safe under a scriptblocker.
* nsXULDocument: Holds strong ref with this patch.
* nsSVGUseElement: Doesn't drop any strong refs.
* nsSVGMpathElement: Doesn't drop any strong refs.
* nsXMLEventsManager: Holds strong ref with this patch.
* nsXMLPrettyPrinter: Already holds a strong ref to itself.
* nsXPathResult: Safe.
* txMozillaXSLTProcessor: Safe.
* nsMenuGroupOwnerX: Holds strong ref with this patch.
* nsDocAccessible: Safe.
* nsSVGEffects: Doesn't drop any strong refs.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 9. ContentAppended.

Why this is safe, broken down by implementation of ContentAppended:

* PresShell: Safe under a script blocker.
* nsTreeContentView: Holds strong ref with this patch.
* nsSVGEffects: Doesn't drop any strong refs.
* inDOMView: Holds strong ref with this patch.
* nsImageMap: Lifetime managed by frame; can't destroy frame.
* nsSHEntry: Already holds a strong ref to self.
* nsScriptElement: Safe under a scriptblocker.
* nsContentList: Safe.
* nsBindingManager: Already called with weak ref.
* nsXULDocument: Holds strong ref with this patch.
* nsSVGStyleElement: Safe under a scriptblocker.
* nsSVGUseElement: Doesn't drop any strong refs.
* nsSVGTitleElement: Doesn't drop any strong refs.
* nsTextStateManager: Doesn't drop any strong refs.
* nsXMLEventsManager: Holds strong ref with this patch.
* nsHTMLTextAreaElement: Already holds a strong ref to self.
* nsAnonDivObserver: Doesn't drop any strong refs.
* nsHTMLStyleElement: Safe under a scriptblocker.
* nsHTMLOutputElement: Doesn't drop any strong refs.
* nsHTMLTitleElement: Doesn't drop any strong refs.
* nsXMLPrettyPrinter: Already holds a strong ref to itself.
* nsXPathResult: Safe.
* txMozillaXSLTProcessor: Safe.
* nsMenuGroupOwnerX: Holds strong ref with this patch.
* nsDocAccessible: Safe.
* nsHTMLEditor: Holds strong ref with this patch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Note: some observers call ContentInserted from ContentAppended, so the changes for ContentAppended cover ContentInserted too for those, if I just added the ref in ContentInserted.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 10. ContentInserted

Why this is safe, broken down by implementation of ContentInserted:

* PresShell: Safe under a script blocker.
* nsTreeContentView: Holds strong ref with this patch.
* nsSVGEffects: Doesn't drop any strong refs.
* inDOMView: Holds strong ref with this patch.
* nsImageMap: Lifetime managed by frame; can't destroy frame.
* nsSHEntry: Already holds a strong ref to self.
* nsScriptElement: Safe under a scriptblocker.
* nsContentList: Safe.
* nsNodeIterator: Doesn't drop any strong refs.
* nsRange: Doesn't drop any strong refs.
* nsBindingManager: Already called with weak ref.
* nsXULDocument: Holds strong ref with this patch.
* nsSVGStyleElement: Safe under a scriptblocker.
* nsSVGUseElement: Doesn't drop any strong refs.
* nsSVGTitleElement: Doesn't drop any strong refs.
* nsTextStateManager: Doesn't drop any strong refs.
* nsXMLEventsManager: Already holds a strong ref to self.
* nsHTMLTextAreaElement: Already holds a strong ref to self.
* nsAnonDivObserver: Doesn't drop any strong refs.
* nsHTMLStyleElement: Safe under a scriptblocker.
* nsHTMLOutputElement: Doesn't drop any strong refs.
* nsHTMLTitleElement: Doesn't drop any strong refs.
* nsXMLPrettyPrinter: Already holds a strong ref to itself.
* nsXPathResult: Safe.
* txMozillaXSLTProcessor: Safe.
* nsMenuGroupOwnerX: Already holds a strong ref to itself.
* nsDocAccessible: Safe.
* nsHTMLEditor: Already holds a strong ref to itself.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 11. ContentRemoved

Why this is safe, broken down by implementation of ContentRemoved:

* PresShell: Safe under a script blocker.
* nsTreeContentView: Holds strong ref with this patch.
* nsSVGEffects: Doesn't drop any strong refs.
* inDOMView: Holds strong ref with this patch.
* nsImageMap: Lifetime managed by frame; can't destroy frame.
* nsSHEntry: Already holds a strong ref to self.
* nsContentList: Safe.
* nsNodeIterator: Doesn't drop any strong refs.
* nsRange: Doesn't drop any strong refs.
* nsBindingManager: Already called with weak ref.
* nsXULTemplateBuilder: Already holds a strong ref to self.
* nsXULDocument: Holds strong ref with this patch.
* nsSVGStyleElement: Safe under a scriptblocker.
* nsSVGUseElement: Doesn't drop any strong refs.
* nsSVGTitleElement: Doesn't drop any strong refs.
* nsTextStateManager: Doesn't drop any strong refs.
* nsXMLEventsManager: Holds strong ref with this patch.
* nsHTMLTextAreaElement: Already holds a strong ref to self.
* nsAnonDivObserver: Doesn't drop any strong refs.
* nsHTMLStyleElement: Safe under a scriptblocker.
* nsHTMLOutputElement: Doesn't drop any strong refs.
* nsHTMLTitleElement: Doesn't drop any strong refs.
* nsXMLPrettyPrinter: Already holds a strong ref to itself.
* nsXPathResult: Safe.
* txMozillaXSLTProcessor: Safe.
* nsMenuGroupOwnerX: Holds strong ref with this patch.
* nsDocAccessible: Safe.
* nsHTMLEditor: Already holds a strong ref to itself.

Comment 13

[Johnny Stenback (:jst)]

Is this about a generic slowness issue or a test in particular where this shows up a bunch?

Comment 14

[Boris Zbarsky [:bzbarsky]]

This shows up at the several percent level (usually 2-3%, but I think I've seen as high as 10%, depending on the test) in pretty much all DOM manipulation testcases.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 457340 [details] [diff] [review]
Part 1.  Add new macrology

So the intent here is to just change macro names, right? Not change any behavior.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 457343 [details] [diff] [review]
Part 4.  No refs when calling NodeWillBeDestroyed

*Might* be worth leaving this as a strong ref given the number of callsites that need to use death-grips. Also considering that the purpose of this call often is for the observer to stop doing whatever it's doing.

OTOH it's nice with consistency.

r=me either way

Comment 17

[Boris Zbarsky [:bzbarsky]]

> So the intent here is to just change macro names, right? Not change any
> behavior.

Yes.

> *Might* be worth leaving this as a strong ref given the number of callsites
> that need to use death-grips. 

There are a lot more that don't (all the subclasses of the stub observers).  I think the consistency and the fact that all those other observers don't need the refs is worth it.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: roll-up patch for approval

This should be pretty safer (I was very conservative about where I held the strong refs) and gets rid of one of our perennial DOM performance annoyances.  Speeds up testcases by 1-10%, depending on the testcase.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Pushed:

  http://hg.mozilla.org/mozilla-central/rev/32d3428169a2
  http://hg.mozilla.org/mozilla-central/rev/a0556636778c
  http://hg.mozilla.org/mozilla-central/rev/b357183c1add
  http://hg.mozilla.org/mozilla-central/rev/f6599baced65
  http://hg.mozilla.org/mozilla-central/rev/94bd54ac2261
  http://hg.mozilla.org/mozilla-central/rev/1a234f884921
  http://hg.mozilla.org/mozilla-central/rev/c2b4791a2700
  http://hg.mozilla.org/mozilla-central/rev/b397d9d1b6c1
  http://hg.mozilla.org/mozilla-central/rev/1c424f1f465e
  http://hg.mozilla.org/mozilla-central/rev/cccefcd4ceda
  http://hg.mozilla.org/mozilla-central/rev/b20c759afb7c