D11487: optimization of TextLineData::attribute

jtamate updated this revision to Diff 29975. jtamate retitled this revision from "simple optimization of TextLineData::attribute" to "optimization of TextLineData::attribute". jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment.

D11487: optimization of TextLineData::attribute

cullmann added a comment. Hmm, does it really get that much faster or is the stuff just moved to the subroutines? I would not have thought that something is faster than simple linear scan for a short vector. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D11487

D11487: optimization of TextLineData::attribute

mwolff added a comment. can you publish the test file and how you measured it? Then I can test it with perf too, to confirm the impact. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D11487 To: jtamate, #frameworks, #kate Cc: mwolff, cullmann, michaelh, kevinapav

D11487: optimization of TextLineData::attribute

jtamate added a comment. In D11487#229889 , @cullmann wrote: > Hmm, does it really get that much faster or is the stuff just moved to the subroutines? The sum of the subroutines time is lower than the routine time as shown in kcachegrind

D11487: optimization of TextLineData::attribute

jtamate added a comment. In D11487#229906 , @mwolff wrote: > can you publish the test file and how you measured it? Then I can test it with perf too, to confirm the impact. I'm sorry, I can't. It contains too much private information to b

D11487: optimization of TextLineData::attribute

cullmann added a comment. I still am not that convinced that binary search helps for vectors which normally have only << 10-20 elements for normal long text lines. But I would be ok with some range based for variant, as the loop with index increment is even ugly without any speed in mind ;=

D11487: optimization of TextLineData::attribute

jtamate updated this revision to Diff 30117. jtamate edited the summary of this revision. jtamate added a comment. The problem with attribute() is that with a long line and word wrapping enabled, when the long line is shown, it is called as many times as the length of the line, in my tes

D11487: optimization of TextLineData::attribute

cullmann added a comment. Ok, that is true, for the "degenerated" very long line case it makes sense to do the binary search. But I don't think you should do that on your own, lower_bound is good enough, we use that for other things like the folding, too, if I am not mistaken. REPOSITORY

D11487: optimization of TextLineData::attribute

mwolff added a comment. yes, definitely don't roll your own lower_bound - use the STL provided one. Are you really compiling in release mode while measuring this? Also, I can only repeat myself in saying that you shouldn't use callgrind for performance measurements anymore, perf/hotspot shou

D11487: optimization of TextLineData::attribute

jtamate added a comment. In D11487#230755 , @mwolff wrote: > yes, definitely don't roll your own lower_bound - use the STL provided one. Are you really compiling in release mode while measuring this? Also, I can only repeat myself in saying that

D11487: optimization of TextLineData::attribute

mwolff added a comment. In D11487#230791 , @jtamate wrote: > In D11487#230755 , @mwolff wrote: > > > yes, definitely don't roll your own lower_bound - use the STL provided one. Are you really compili

D11487: optimization of TextLineData::attribute

jtamate updated this revision to Diff 30134. jtamate added a comment. > Huh, just specify -DCMAKE_INSTALL_PREFIX to the same path where you install the KF5 devel packages to, as a simple fix. Thanks, that worked. > Are you really compiling in release mode while measuring this? Y

D11487: optimization of TextLineData::attribute

mwolff added a subscriber: dhaumann. mwolff added a comment. Looking at `TextLineData::addAttribute`, it doesn't seem to sort the data - how can you be sure that the attributes list really is sorted by `offset + length`? I think this should be done manually there, or at least asserted. The o

D11487: optimization of TextLineData::attribute

jtamate added a comment. In D11487#230895 , @mwolff wrote: > Looking at `TextLineData::addAttribute`, it doesn't seem to sort the data - how can you be sure that the attributes list really is sorted by `offset + length`? I think this should be d

D11487: optimization of TextLineData::attribute

jtamate added a comment. Uploaded a similar file F5761614: fake.xml.gz It is created using http://interglacial.com/~sburke/pub/rtf2xml.html from a rtf with two images and some text. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.k

D11487: optimization of TextLineData::attribute

jtamate added a comment. I'm sorry, the previous file lines are 10 times longer than expected. This one {F5761669 }can be managed by kate, not only by okteta. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D11487 To: jtamat

D11487: optimization of TextLineData::attribute

mwolff added a comment. In D11487#230913 , @jtamate wrote: > In D11487#230895 , @mwolff wrote: > > > Looking at `TextLineData::addAttribute`, it doesn't seem to sort the data - how can you be sure th

D11487: optimization of TextLineData::attribute

mwolff added a comment. When I open your fake file in kwrite, then raise the line length limit and wait for the file to be rendered (which takes quite some time!), perf does not show any performance issues with attribute for me. It only corresponds to ~1.3% of the CPU cycles. `QTextLine::lay

D11487: optimization of TextLineData::attribute

mwolff added a comment. Considering spell checking is involved - can you show a screenshot for how the file looks like for you? There shouldn't be a lot of spell checking going on, or so I hope... REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D11487 To: jtamate

D11487: optimization of TextLineData::attribute

cullmann added a comment. For the sorting/overlapping question: actually the HL code should ensure there are neither overlaps nor unsorted entries. And I think if there are, already now "strange" things happen, as normally the first item would win. REPOSITORY R39 KTextEditor REVISION DE

D11487: optimization of TextLineData::attribute

anthonyfieroni added inline comments. INLINE COMMENTS > katetextline.cpp:214-216 > +if (first != m_attributesList.cend() && (*first).offset <= pos && pos < > ((*first).offset + (*first).length)) > +{ > +return (*first).attributeValue; Use operator->, it's faster than operator* a

D11487: optimization of TextLineData::attribute

jtamate added a comment. In D11487#231110 , @mwolff wrote: > Considering spell checking is involved - can you show a screenshot for how the file looks like for you? There shouldn't be a lot of spell checking going on, or so I hope... F57

D11487: optimization of TextLineData::attribute

mwolff added inline comments. INLINE COMMENTS > anthonyfieroni wrote in katetextline.cpp:214-216 > Use operator->, it's faster than operator* and operator. > > first->offset sorry, but that's simply not true at all. Stylistic wise I agree, but an optimizing compiler will generate the same co

D11487: optimization of TextLineData::attribute

mwolff added a comment. @jtamate looking at your screenshots, it represents closely what I see locally. Most notably, there are no red underlines in your screenshots which could arise due to spell checking. Thus I really wonder why you are seeing such a big hotspot there. Try perf, or t

D11487: optimization of TextLineData::attribute

jtamate added a comment. In D11487#231522 , @mwolff wrote: > @jtamate looking at your screenshots, it represents closely what I see locally. Most notably, there are no red underlines in your screenshots which could arise due to spell checking. T

D11487: optimization of TextLineData::attribute

mwolff added a comment. In D11487#231656 , @jtamate wrote: > In D11487#231522 , @mwolff wrote: > > > @jtamate looking at your screenshots, it represents closely what I see locally. Most notably, ther

D11487: optimization of TextLineData::attribute

mwolff added a comment. Also, what is "@mwolf solution" - I didn't provide any code, until now: diff --git a/src/spellcheck/spellcheck.cpp b/src/spellcheck/spellcheck.cpp index 9e04d788..3a97e5c5 100644 --- a/src/spellcheck/spellcheck.cpp +++ b/src/spellcheck/spellcheck.cpp

D11487: optimization of TextLineData::attribute

jtamate added a comment. > One question to that though: Why do you sort/lookup by `x.offset + x.length <= p`? Note how lower_bound returns the first iterator that is _not_ going to return true. Assuming there are neither overlaps nor unsorted entries. Lets call X the iterator returned

D11487: optimization of TextLineData::attribute

mwolff added a comment. In D11487#232258 , @jtamate wrote: > > One question to that though: Why do you sort/lookup by `x.offset + x.length <= p`? Note how lower_bound returns the first iterator that is _not_ going to return true. > > Assumin

D11487: optimization of TextLineData::attribute

jtamate updated this revision to Diff 30331. jtamate edited the summary of this revision. jtamate added a comment. Using upper_bound with < Changed the name of the iterator Using -> instead of (*x) These are the calls I get since I open the document: doc= KTextEditor::DocumentPrivate

D11487: optimization of TextLineData::attribute

mwolff added a comment. +1 from my side, @cullmann, @dhaumann ? I'll continue to figure out why I don't see the performance issue here REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D11487 To: jtamate, #frameworks, #kate Cc: anthonyfieroni, dhaumann, mwolff,

D11487: optimization of TextLineData::attribute

cullmann accepted this revision. cullmann added a comment. This revision is now accepted and ready to land. Yep, makes sense. Thanks for the improvement. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D11487 To: jtamate, #frameworks, #kate, cullmann Cc: anthonyf

D11487: optimization of TextLineData::attribute

This revision was automatically updated to reflect the committed changes. jtamate marked 2 inline comments as done. Closed by commit R39:787318967fce: optimization of TextLineData::attribute (authored by jtamate). REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.o

D11487: optimization of TextLineData::attribute

mwolff added a comment. @jtamate I just checked, the function is called with the same parameters for me locally. What output do you get for this: diff --git a/src/spellcheck/spellcheck.cpp b/src/spellcheck/spellcheck.cpp index 9e04d788..50e92885 100644 --- a/src/spellcheck/spell

D11487: optimization of TextLineData::attribute

jtamate added a comment. In D11487#232497 , @mwolff wrote: > @jtamate I just checked, the function is called with the same parameters for me locally. What output do you get for this: > > For me this is the interesting bit: > > 15.097 de

D11487: optimization of TextLineData::attribute

mwolff added a comment. WTF :D Your desktop CPU has clearly a better performance than my mobile CPU, no? Is AMD really so much worse here? How can that be - I don't get it :D Anyhow, I give up trying to understand this now - thanks a lot for your repeated input Jaime! REPOSITORY R39 K

D11487: optimization of TextLineData::attribute

jtamate added a comment. In D11487#232555 , @mwolff wrote: > WTF :D Your desktop CPU has clearly a better performance than my mobile CPU, no? Is AMD really so much worse here? How can that be - I don't get it :D > > Anyhow, I give up trying t

D11487: optimization of TextLineData::attribute

mwolff added a comment. Ah, I knew it :D I suspected missing compiler optimizations from the start... But tricky indeed for you to see! But the real question would now be: what time does it take *without* this patch for you? probably it won't be more than a minute anymore. Still, this p