Optimize nsTransactionManager traversing

RESOLVED FIXED in mozilla12

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 1 bug)

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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

6 years ago
mRootElement check wouldn't help with full page editors, but need to think about something else
for them.
(Assignee)

Comment 2

6 years ago
Created attachment 584940 [details] [diff] [review]
patch

Something like this.

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

Updated

6 years ago
Assignee: nobody → bugs
Component: DOM → Editor
QA Contact: general → editor
(Assignee)

Comment 3

6 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)
Blocks: 698919
OS: Linux → All
Hardware: x86_64 → All
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 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

6 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.
(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

6 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

6 years ago
https://hg.mozilla.org/mozilla-central/rev/f268da52217d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Version: 10 Branch → Trunk
Blocks: 716598
No longer blocks: 698919
You need to log in before you can comment on or make changes to this bug.