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)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

      No description provided.
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%.
Blocks: 1346723, 1358025
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!
This avoids the cost of allocation on the heap and cycle collection,
and removes some dead code.
Attachment #8891603 - Flags: review?(masayuki)
With the second part here, I get a couple of milliseconds lower minimum time on the test case.
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 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+
(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...
(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!
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
Depends on: 1386222
Depends on: 1386960
Assignee: nobody → ehsan
You need to log in before you can comment on or make changes to this bug.