D17441: tune editing actions for large number of small edits

cullmann added a comment. In D17441#374925 , @ngraham wrote: > In D17441#374924 , @dhaumann wrote: > > > I guess it's ok to have this in. But please use this version at your daily work ;) I think we

D17441: tune editing actions for large number of small edits

ngraham added a comment. In D17441#374924 , @dhaumann wrote: > I guess it's ok to have this in. But please use this version at your daily work ;) I think we don't have many testers... I always use Kate from git master. Is there anything i

D17441: tune editing actions for large number of small edits

dhaumann added a comment. I guess it's ok to have this in. But please use this version at your daily work ;) I think we don't have many testers... REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17441 To: cullmann, dhaumann, #kate, loh.tar Cc: anthonyfieroni, kwr

D17441: tune editing actions for large number of small edits

cullmann added a comment. Fixed my QSet issues, I was just stupid, I shall have guarded the push_back with the bool, not the later use. ;=) REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17441 To: cullmann, dhaumann, #kate, loh.tar Cc: anthonyfieroni, kwrite-dev

D17441: tune editing actions for large number of small edits

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit R39:2138e1d27c83: improve range handling, no allocations for common updates (authored by cullmann). CHANGED PRIOR TO COMMIT

D17441: tune editing actions for large number of small edits

cullmann added a comment. In D17441#373939 , @loh.tar wrote: > > Yes, there is now a limit for how many highlightings are done. > > If its too much, we skip that, else the rendering will break everthing down ;=) > > Doing these highliting

D17441: tune editing actions for large number of small edits

loh.tar added a comment. > Yes, there is now a limit for how many highlightings are done. > If its too much, we skip that, else the rendering will break everthing down ;=) Doing these highliting "on the fly" (only what is/became visible) is probably not possible(?) BTW: Thanks a

D17441: tune editing actions for large number of small edits

cullmann added a comment. > The only curiosity I noticed is that no highliting take effect when there are too much hits(?) Search some number like 5 "142926 matches found" no highlight. Char A "10565 matches found" is highlighted Yes, there is now a limit for how many highlightings are d

D17441: tune editing actions for large number of small edits

loh.tar added a comment. > This needs now some user testing ;=) Done as previous, with that testfile. Seams to work so far. Find is still quick, these TAB S&R takes around 2min. The only curiosity I noticed is that no highliting take effect when there are too much hits(?) Search som

D17441: tune editing actions for large number of small edits

cullmann reopened this revision. cullmann added a comment. Hmm, the keys test seem to fail not for all Qt versions. Qt 5.9 ok Qt 5.11 bad see e.g. CI. Before and after this commit... REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17441 To: cullmann, dhaum

D17441: tune editing actions for large number of small edits

cullmann added a comment. Hmpf, I amended my commit message to contain a CHANGELOG... and stuff bug unfortunately arc land ate that again :P REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17441 To: cullmann, dhaumann, #kate, loh.tar Cc: anthonyfieroni, kwrite-de

D17441: tune editing actions for large number of small edits

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit R39:85a1c56ed832: tune editing actions for large number of small edits (authored by cullmann). REPOSITORY R39 KTextEditor

D17441: tune editing actions for large number of small edits

cullmann added a comment. This needs now some user testing ;=) REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17441 To: cullmann, dhaumann, #kate, loh.tar Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, demsking, cullmann,

D17441: tune editing actions for large number of small edits

cullmann added inline comments. INLINE COMMENTS > loh.tar wrote in katetextblock.cpp:290 > Noob question: Would QPointer help? > > if (range && range->isValidityCheckRequired()) { As it is no QObject, no ;=) Actually, I think all workaround might be even more expensive than the QSet. REPOSI

D17441: tune editing actions for large number of small edits

cullmann added a comment. One issue I have with the auto-tests: With and without any of these changes here locally the vimode_keys test fails. FAIL! : KeysTest::MacroTests() Compared values are not the same Actual (kate_document->text()): "completionMacro completionRepeatLa

D17441: tune editing actions for large number of small edits

loh.tar added inline comments. INLINE COMMENTS > cullmann wrote in katetextblock.cpp:290 > Ok, doesn't work due to the issue that we might delete the range via notifier > in checkValidity() :P Noob question: Would QPointer help? if (range && range->isValidityCheckRequired()) { REPOSITORY

D17441: tune editing actions for large number of small edits

cullmann marked 6 inline comments as done. cullmann added a comment. Crash fixed again. Updated comments to make more clear what happens. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17441 To: cullmann, dhaumann, #kate, loh.tar Cc: anthonyfieroni, kwrite-deve

D17441: tune editing actions for large number of small edits

cullmann updated this revision to Diff 47181. cullmann added a comment. go back to QSet for the checkValidity, mark in the comments that ranges might be deleted REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17441?vs=47177&id=47181 BRANCH arcpatch-D1

D17441: tune editing actions for large number of small edits

cullmann added a comment. Ok, the optimization with the vector fails due delets inside checkValidity(), need to revert that and update docs. INLINE COMMENTS > cullmann wrote in katetextblock.cpp:290 > This loop now crash in the unit test > > ./bin/bug313759 > > ) > = > > It works again, i

D17441: tune editing actions for large number of small edits

cullmann added a comment. :P Crash for the win. INLINE COMMENTS > katetextblock.cpp:290 > +// we might need to invalidate ranges or notify about their changes > +for (TextRange *range : changedRanges) { > +if (range->isValidityCheckRequired()) { This loop now cras

D17441: tune editing actions for large number of small edits

cullmann updated this revision to Diff 47177. cullmann added a comment. save 1% total runtime in addItem REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17441?vs=47174&id=47177 BRANCH arcpatch-D17441 REVISION DETAIL https://phabricator.kde.org/D1744

D17441: tune editing actions for large number of small edits

cullmann updated this revision to Diff 47174. cullmann added a comment. next minor bottleneck: QList with things a bit larger than a pointer in addition, here zero sharing is needed or wanted, we can use a plain vector REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabric

D17441: tune editing actions for large number of small edits

cullmann updated this revision to Diff 47173. cullmann added a comment. avoid most allocations for the common cases of a few affected ranges REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17441?vs=47172&id=47173 BRANCH arcpatch-D17441 REVISION DETAIL

D17441: tune editing actions for large number of small edits

cullmann updated this revision to Diff 47172. cullmann added a comment. improve m_isCheckValidityRequired management REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17441?vs=47160&id=47172 BRANCH arcpatch-D17441 REVISION DETAIL https://phabricator.k

D17441: tune editing actions for large number of small edits

cullmann added a comment. In D17441#373728 , @loh.tar wrote: > I have tried this one. Yes, works much more better! > Nice to see that my own (unloved) patch wake your interest to dig into that issue :-) :=) Nice that it helps a bit fo

D17441: tune editing actions for large number of small edits

cullmann added a comment. For the unit test: we have tests for ranges, it makes no sense to test if the bool is set. As long as the range tests still work, the updates are ok. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17441 To: cullmann, dhaumann, #kate,

D17441: tune editing actions for large number of small edits

cullmann added a comment. Will improve further ;=) INLINE COMMENTS > anthonyfieroni wrote in katetextblock.cpp:153 > You can use unordered_set, so you don't need m_isCheckValidityRequired any > more. It can be applied to other places as well. Actually no, that will be again a lot more expen

D17441: tune editing actions for large number of small edits

dhaumann added inline comments. INLINE COMMENTS > katetextrange.h:364 > + */ > +bool m_isCheckValidityRequired = false; > }; I would prefer an additional inline void setValidityCheckRequired(); inline bool validityCheckRequired() const; We don't need a bool in the setter, since we

D17441: tune editing actions for large number of small edits

anthonyfieroni added inline comments. INLINE COMMENTS > katetextblock.cpp:153 > // remember all ranges modified > -QSet changedRanges; > +std::vector changedRanges; > foreach (TextCursor *cursor, m_cursors) { You can use unordered_set, so you don't need m_isCheckValidityRequire

D17441: tune editing actions for large number of small edits

loh.tar added a comment. I have tried this one. Yes, works much more better! Nice to see that my own (unloved) patch wake your interest to dig into that issue :-) As usual understand I most of your talk here not really, and these perf protocols give me no light on the first sight. But

D17441: tune editing actions for large number of small edits

cullmann added a comment. 85.51% 0.00% kwrite kwrite [.] main 85.49% 0.00% kwrite libc-2.28.so[.] __libc_start_main 85.47% 0.00% kwrite kwrite [.] _start

D17441: tune editing actions for large number of small edits

cullmann updated this revision to Diff 47160. cullmann added a comment. save expensive buildReplacement call if no place holders around REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17441?vs=47159&id=47160 BRANCH master REVISION DETAIL https://pha

D17441: tune editing actions for large number of small edits

cullmann added a comment. 2.54% 0.10% kwrite libKF5TextEditor.so.5.53.0 [.] KTextEditor::DocumentCursor::atEndOfDocument :P ROFL REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17441 To: cullmann, dhaumann, #kate, loh.tar Cc: kwrite-d

D17441: tune editing actions for large number of small edits

cullmann updated this revision to Diff 47159. cullmann added a comment. avoid costly lineLength computation this really is expensive, as we do a lookup of the line that kills the cache in the block lookup + we can skip that easily for non-last lines costs 2% for large replaces REPO

D17441: tune editing actions for large number of small edits

cullmann added a comment. New state: 85.24% 0.00% kwrite kwrite [.] main 85.21% 0.00% kwrite libc-2.28.so[.] __libc_start_main 85.19% 0.00% kwrite kwrite

D17441: tune editing actions for large number of small edits

cullmann updated this revision to Diff 47157. cullmann added a comment. only update edit position stack once per transaction with last position of any primitive REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17441?vs=47155&id=47157 BRANCH master REV

D17441: tune editing actions for large number of small edits

cullmann added a comment. Current profile F6464730: perf.zsynb.svg REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17441 To: cullmann, dhaumann, #kate, loh.tar Cc: kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham

D17441: tune editing actions for large number of small edits

cullmann updated this revision to Diff 47155. cullmann added a comment. avoid massive moving range creation use now 1 moving range, not 1 per replacement REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17441?vs=47151&id=47155 BRANCH master REVISI

D17441: tune editing actions for large number of small edits

cullmann updated this revision to Diff 47151. cullmann added a comment. avoid a lot of allocations for the sets we can just use a vector and remember if we need to update a range, in worst case, twice inserted REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.

D17441: tune editing actions for large number of small edits

cullmann added a comment. :=) at least we now beat atom, with the test case file and replace all \t >= ; it just crashed here after "non responding". REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17441 To: cullmann, dhaumann, #kate, loh.tar Cc: kwrite-devel, kd

D17441: tune editing actions for large number of small edits

cullmann added a comment. Same issue with the marks of the vimode. Perhaps they should not triggered at all if vi mode is off ;=) In any case, this test shows a lot of bottlenecks. e.g. even the l->toVirtualColumn that is used for block mode did show up like hell because it was accidentl

D17441: tune editing actions for large number of small edits

dhaumann added a comment. Hm indeed. I remember when the patch was added that added the editing positions. At that time I was not convinced about this, and now even performance problems show up. What I wonder is: Usually, these editing positions should just be the ones the user really j

D17441: tune editing actions for large number of small edits

cullmann added a comment. Yes, even after this optimization :P 10% Here is the flat profile, after all this (using the test case from the bug, replacing \t with ; and terminating kwrite afterwards) 86.90% 2.05% kwrite libQt5Core.so.5.11.2[.] QMetaObj

D17441: tune editing actions for large number of small edits

dhaumann added a comment. Just curious: Do the editing positions really show up in perf? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17441 To: cullmann, dhaumann, #kate, loh.tar Cc: kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, demsking,

D17441: tune editing actions for large number of small edits

cullmann added a reviewer: loh.tar. cullmann added a comment. Could you try this patch? With it, the test case from the bug properly works with < 3GB of memory. It is still VERY slow, but it doesn't lead to an unusable state. Before, alone the replacement moving ranges that are used for t

D17441: tune editing actions for large number of small edits

cullmann updated this revision to Diff 47150. cullmann added a comment. limit number of highlightings for replace more than 2^16 things will not be marked REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17441?vs=47148&id=47150 BRANCH master REVISION

D17441: tune editing actions for large number of small edits

cullmann added reviewers: dhaumann, Kate. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17441 To: cullmann, dhaumann, #kate Cc: kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17441: tune editing actions for large number of small edits

cullmann created this revision. Herald added projects: Kate, Frameworks. Herald added subscribers: kde-frameworks-devel, kwrite-devel. cullmann requested review of this revision. REVISION SUMMARY example workload that triggers these bottlenecks: mass replace of a character in a large document