Closed
Bug 1385525
Opened 7 years ago
Closed 7 years ago
Speed up EditorBase::SetTextImpl() by actually bypassing all of the editor transaction management machinery
Categories
(Core :: DOM: Editor, enhancement)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
1.20 KB,
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
9.19 KB,
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8891586 -
Flags: review?(masayuki)
Assignee | ||
Comment 2•7 years ago
|
||
Technically the API requires us to call EditorBase::DoTransaction() as the code currently does, however the whole goal of bug 1358025 was to not create transactions here, yet we are still instantiating all of the required setup for them. We can actually cheat by bypassing all of this setup and call into the transaction object directly.
The transaction object has no way of knowing who called it, so with a well designed object oriented system, this technique should always be expected to work even!
On my machine, this patch alone speeds up the test case of bug 1358025 by more than 20%.
Assignee | ||
Comment 3•7 years ago
|
||
Actually, I realized that now there is no object that holds a reference to SetTextTransaction any more, so I can just make it a nice stack class and avoid a whole bunch of malloc and cycle collector overhead in one shot as well!
Assignee | ||
Comment 4•7 years ago
|
||
This avoids the cost of allocation on the heap and cycle collection,
and removes some dead code.
Attachment #8891603 -
Flags: review?(masayuki)
Assignee | ||
Comment 5•7 years ago
|
||
With the second part here, I get a couple of milliseconds lower minimum time on the test case.
Comment 6•7 years ago
|
||
Comment on attachment 8891586 [details] [diff] [review]
Speed up EditorBase::SetTextImpl() by actually bypassing all of the editor transaction management machinery
EditorBase::DoTransaction() calls EditorBase::DoAfterDoTransaction(), then, SetTextTransaction::GetIsTransient() (perhaps) returns true and EditorBase::DoAfterDoTransaction() calls IncrementModificationCount(1), then, finally, EditorBase::IncrementModificationCount() *may* call NotifyDocumentListeners(eDocumentStateChanged). So, I think that you should call IncrementModificationCount(1) after calling transaction->DoTransaction() and it returns NS_OK.
Although, I think that rather than calling SetTextTransaction::DoTransaction() directly here, EditorBase::DoTransaction() should no do unnecessary work when undo is disabled because like the issue of IncrementModificationCount(), other things would be broken only when setting the value.
However, that must be risky for now and fixed later.
Attachment #8891586 -
Flags: review?(masayuki) → review+
Comment 7•7 years ago
|
||
Comment on attachment 8891603 [details] [diff] [review]
Part 2: Make SetTextTransaction lighter weight by making it a normal C++ class allocated on the stack
I guess that we can make mEditorBase to |EditorBast* MOZ_NON_OWNING_REF| but up to you.
Attachment #8891603 -
Flags: review?(masayuki) → review+
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> Comment on attachment 8891586 [details] [diff] [review]
> Speed up EditorBase::SetTextImpl() by actually bypassing all of the editor
> transaction management machinery
>
> EditorBase::DoTransaction() calls EditorBase::DoAfterDoTransaction(), then,
> SetTextTransaction::GetIsTransient() (perhaps) returns true
FWIW there are no transient transactions in m-c, this always returns false. (We should consider removing GetIsTransient IMO.)
> and
> EditorBase::DoAfterDoTransaction() calls IncrementModificationCount(1),
> then, finally, EditorBase::IncrementModificationCount() *may* call
> NotifyDocumentListeners(eDocumentStateChanged). So, I think that you should
> call IncrementModificationCount(1) after calling
> transaction->DoTransaction() and it returns NS_OK.
Sorry I didn't explain this part. This is the ultimate side effect of the NotifyDocumentListeners() call: https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/editor/composer/nsComposerCommandsUpdater.cpp#254
It's actually a mistake to run this code when we aren't adding an undo/redo entry, since this updates the command state to reflect that there is something to undo (for example by enabling a UI undo button if there is one) and if the user then presses such a button nothing will happen. So I intentionally chose to not change the modification count here.
> Although, I think that rather than calling
> SetTextTransaction::DoTransaction() directly here,
> EditorBase::DoTransaction() should no do unnecessary work when undo is
> disabled because like the issue of IncrementModificationCount(), other
> things would be broken only when setting the value.
>
> However, that must be risky for now and fixed later.
I'm not really sure about this part of your comment. All of the DoTransaction() code sort of assumes that undo/redo is always available, since that is the entire reason why you would want to use the transaction manager. Indeed what we are doing here is really quite an exceptional case, I can't think of anywhere else in the editor code where we have any similar behavior off the top of my head...
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #7)
> Comment on attachment 8891603 [details] [diff] [review]
> Part 2: Make SetTextTransaction lighter weight by making it a normal C++
> class allocated on the stack
>
> I guess that we can make mEditorBase to |EditorBast* MOZ_NON_OWNING_REF| but
> up to you.
Yes, should be safe to do that now, good idea!
Comment 10•7 years ago
|
||
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df5e7bd8b673
Part 1: Speed up EditorBase::SetTextImpl() by actually bypassing all of the editor transaction management machinery; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb4439aa194
Part 2: Make SetTextTransaction lighter weight by making it a normal C++ class allocated on the stack; r=masayuki
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/df5e7bd8b673
https://hg.mozilla.org/mozilla-central/rev/dfb4439aa194
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•6 years ago
|
Assignee: nobody → ehsan
You need to log in
before you can comment on or make changes to this bug.
Description
•