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] (pto-ish for couple of days)
:
: Makoto Kato [:m_kato]
Mentors:
Depends on:
Blocks: 716598
  Show dependency treegraph
 
Reported: 2011-12-30 03:11 PST by Olli Pettay [:smaug] (pto-ish for couple of days)
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] (pto-ish for couple of days)
ehsan: review+
continuation: review+
Details | Diff | Splinter Review

Description User image Olli Pettay [:smaug] (pto-ish for couple of days) 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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 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 User image :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 User image 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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 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 User image 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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 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.