Review Request: Add a 10ms timer to scheduling a document for reparsing, and refactor -- fixes slowness when inserting text

[Andreas Pakulat]

> On Oct. 31, 2012, 7:55 a.m., Andreas Pakulat wrote:
> > Can you explain a bit more why pasting big chunks is a problem in how the timer helps with that? The problem I'm seeing is that you're making some code asynchronous that was synchronous before, this can have nasty side-effects if there are unwritten assumptions about this elsewhere.
> 
> Sven Brauch wrote:
>     The BackgroundParser::addDocument function is being called whenever the textChanged signal is emitted. This happens a large amount of times if you paste a chunk. Until now, addDocument added an entry to the parse plan whenever it was called, those were only cleared when the background parser actually triggered. The latter happened about 500ms later (the usual background-parser delay). For every call of addDocument, there were serveral places where all entries in the parse plan were scanned and compared. The whole thing thus had ~O(n²) complexity for a large amount of simultaneous insertions, which killed performance. With the suggested changes, there's a) significantly less calls to addDocument and b) no duplicate entries in the parse plan. They will thus reduce complexity to O(n), with a smaller constant factor even.
>     
>     The asynchronous thing you mentioned is indeed something one might need to think about. However, I don't see a place where this would cause problems yet; because in the next step of the whole parse process, everything is made asynchronous anyways because of the backgroundparser delay. Maybe it would be necessary to check in the timer-called function whether the document has been closed or so? I'm not sure.
> 
> Sven Brauch wrote:
>     Oh, if you're extremely worried about the timer thing we can just remove it and just keep the set change. It'll be slower, but still a major performance boost over the previous solution.
> 
> Andreas Pakulat wrote:
>     Do you know why pasting doing a single paste causes multiple textChanged signals? Does this maybe only happen when having Kate configured to reformat source code pasted from the clipboard? To me it seems illogical/wrong that textChanged is emitted more than once for a single change like pasting something and I'd rather try to fix that.
> 
> Sven Brauch wrote:
>     No, I don't. And no, I have the indent code for pastes disabled, so it probably doesn't have anything to do with it...
>     And I agree that it seems weird...  but code for, for example, code folding seems to rely on things like "for each line inserted, a "line inserted" signal is emitted".
>     Maybe we should ask the kate people about that? I'll see if I can reach anyone in IRC.
>     
>     Anyways, the thing with the set should go in in any case, since it makes absolutely no sense that the parseplan contains two identical items.

Yes, if the plan is not supposed to contain the same item twice (can't comment on that, I have no clue about that stuff) and items are frequently looked up in it, then a set makes complete sense.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107140/#review21190
-----------------------------------------------------------


On Oct. 30, 2012, 11:14 p.m., Sven Brauch wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107140/
> -----------------------------------------------------------
> 
> (Updated Oct. 30, 2012, 11:14 p.m.)
> 
> 
> Review request for KDevelop and Milian Wolff.
> 
> 
> Description
> -------
> 
>     Add a 10ms timer to scheduling a document for reparsing, and refactor.
>     
>     This fixes extreme slowness when pasting large parts of text
>     into the editor.
>     Also, make DocumentParsePlan targets a set instead of a list.
> 
> I wanted to write a unit test benchmark which verifies this but it turns out to be difficult, since you need a KateTextDocument for inserting text, and it's not being installed (the .h file). I thus didn't do that yet.
> 
> You can easily test this manually by pasting > 3000 lines into the editor.
> 
> 
> Diffs
> -----
> 
>   language/backgroundparser/backgroundparser.cpp 28a4202 
>   language/backgroundparser/documentchangetracker.h e3f7f5c 
>   language/backgroundparser/documentchangetracker.cpp 2aab705 
> 
> Diff: http://git.reviewboard.kde.org/r/107140/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sven Brauch
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20121031/dccd4e9a/attachment.html>