714250 - Optimize nsTransactionManager traversing

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

We seem to traverse huge number of EditTxn objects and they are kept alive by 
nsTransactionManager object.
nsEditor object has nsTransactionManager as member variable, and based on 
CC logs it is nsEditor keeping the object alive.

I think we don't need to traverse nsTransactionManager at all if nsEditor::mRootElement
is in CCGeneration document, since that keeps the editor alive anyway.

Ehsan, can you think of cases when mRootElement is in document, but
editor should be still deleted?

Comment 1

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

mRootElement check wouldn't help with full page editors, but need to think about something else
for them.

Comment 2

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

Attachment: patch

Something like this.

https://tbpl.mozilla.org/?tree=Try&rev=0b94c6c87e74

Comment 3

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

Comment on attachment 584940 [details] [diff] [review]
patch

so far tryserver looks ok.

The patch does not help with transactionmanager used by places, but
in the cases I've seen transactionmanager in CC logs it has been owned by
editor.

Comment 4

[(no longer active)]

Comment on attachment 584940 [details] [diff] [review]
patch

r=me, noting that this won't cover some of the HTML editor cases, but that should be fine...

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 584940 [details] [diff] [review]
patch

Review of attachment 584940 [details] [diff] [review]:
-----------------------------------------------------------------

I do wonder if we could dynamically check some of these ownership relations we are assuming with assertions in these various patches you have, Olli, but maybe that isn't always easy to do.

Comment 6

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

It would be great to have some assertions somewhere to verify that there will be no regressions, but that is hard.

Anyway, I think I've done with all the easy checks (I have still one in my local patch).
I've disabled trace_all for nsICycleCollectorListener, and when creating the log, it is
almost only JS stuff.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #6)
> It would be great to have some assertions somewhere to verify that there
> will be no regressions, but that is hard.

I think the model for this may be nsWrapperCache, or some related class, that checks to see if a certain field is traversed.  It does this by creating a special listener, then calling the traverse function to see if the particular field is reached or not.  Something similar could be done here.

> Anyway, I think I've done with all the easy checks (I have still one in my
> local patch).
> I've disabled trace_all for nsICycleCollectorListener, and when creating the
> log, it is
> almost only JS stuff.

Great!  Is that with the async de-purple patch?

Comment 8

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

(In reply to Andrew McCreight [:mccr8] from comment #7)
> I think the model for this may be nsWrapperCache, or some related class,
But in this case this is all C++

> Something similar could be done
> here.
Ah, perhaps.


> Great!  Is that with the async de-purple patch?
yes.
Well, there is lots of JS and some XPConnect stuff, at least in the logs I've got.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/f268da52217d