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%.

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.

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.

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.

(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

https://hg.mozilla.org/mozilla-central/rev/df5e7bd8b673 https://hg.mozilla.org/mozilla-central/rev/dfb4439aa194