Last Comment Bug 714250 - Optimize nsTransactionManager traversing
: Optimize nsTransactionManager traversing
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on:
Blocks: 716598
  Show dependency treegraph
 
Reported: 2011-12-30 03:11 PST by Olli Pettay [:smaug]
Modified: 2012-01-09 10:42 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.87 KB, patch)
2011-12-30 03:35 PST, Olli Pettay [:smaug]
ehsan: review+
continuation: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2011-12-30 03:11:42 PST
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] 2011-12-30 03:31:20 PST
mRootElement check wouldn't help with full page editors, but need to think about something else
for them.
Comment 2 Olli Pettay [:smaug] 2011-12-30 03:35:41 PST
Created attachment 584940 [details] [diff] [review]
patch

Something like this.

https://tbpl.mozilla.org/?tree=Try&rev=0b94c6c87e74
Comment 3 Olli Pettay [:smaug] 2011-12-30 04:56:47 PST
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 :Ehsan Akhgari 2011-12-30 08:27:27 PST
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] 2011-12-30 08:46:24 PST
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] 2011-12-30 09:10:15 PST
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] 2011-12-30 09:34:17 PST
(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] 2011-12-30 09:36:24 PST
(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] 2011-12-30 09:57:50 PST
https://hg.mozilla.org/mozilla-central/rev/f268da52217d

Note You need to log in before you can comment on or make changes to this bug.