Closed
Bug 714250
Opened 13 years ago
Closed 13 years ago
Optimize nsTransactionManager traversing
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: smaug, Assigned: smaug)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
ehsan.akhgari
:
review+
mccr8
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•13 years ago
|
||
mRootElement check wouldn't help with full page editors, but need to think about something else
for them.
Assignee | ||
Comment 2•13 years ago
|
||
Something like this.
https://tbpl.mozilla.org/?tree=Try&rev=0b94c6c87e74
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → bugs
Component: DOM → Editor
QA Contact: general → editor
Assignee | ||
Comment 3•13 years ago
|
||
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.
Attachment #584940 -
Flags: review?(ehsan)
Attachment #584940 -
Flags: review?(continuation)
Updated•13 years ago
|
Comment 4•13 years ago
|
||
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...
Attachment #584940 -
Flags: review?(ehsan) → review+
Comment 5•13 years ago
|
||
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.
Attachment #584940 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 6•13 years ago
|
||
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•13 years ago
|
||
(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?
Assignee | ||
Comment 8•13 years ago
|
||
(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.
Assignee | ||
Comment 9•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Version: 10 Branch → Trunk
You need to log in
before you can comment on or make changes to this bug.
Description
•