Review Request: Project model speedups for project files with lots of directories and files.

[Milian Wolff]

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


I think the »const KUrl &« look fine. Same for the removal of the dynamic casts.

But for the hash I'm a bit hesitant to accept it in the current format:

1) first of all, the addToFileSet would have to be called one for every item <-> url pair, and not only once for every url. I don't know whether that's actually the case and should be tested. It could mean that e.g. items in targets are not to be found anymore (same url, different item).

2) I think it would be much more elegant to listen to the signals of the ProjectModel and build the hash accordingly, and it should be KUrl <-> BaseItem, not IndexedString <-> BaseItem. The latter is expensive (i.e. turning a KUrl into a IndexedString). Imo the existing model signals should be enough to keep the cache updated properly, no?

Anyways, thanks for the patch. If you don't want to work on the latter part, I'll take a shot at it the next days. But I'd of course warmly welcome any contributions from your side :)

I'll now try to valgrind kdev with the kernel + mesa with and without your changes.

Bye


/trunk/extragear/sdk/kdevplatform/shell/project.cpp
<http://reviewboard.kde.org/r/3722/#comment4640>

    please no whitespace changes



/trunk/extragear/sdk/kdevplatform/shell/project.cpp
<http://reviewboard.kde.org/r/3722/#comment4641>

    If this stays in as-is, it should imo rather be a d->fileHash.remove(file); and remove the url for every item, making the extra argument obsolete.
    
    I mean it gets removed from the fileSet, hence it shouldn't be referenced in the other set either.


- Milian


On 2010-04-21 16:34:36, Brandon Ehle wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3722/
> -----------------------------------------------------------
> 
> (Updated 2010-04-21 16:34:36)
> 
> 
> Review request for KDevelop.
> 
> 
> Summary
> -------
> 
> My test case consists of 3 projects (Linux kernel main tree, DRM tree, and Mesa tree).
> 
> 
> Changes:
> 
> 1) Replace recursion inside itemsForUrl with a QMultiHash lookup to speedup itemsForUrl() on large projects.
> 
> 2) Change the url() function for the ProjectBaseItem to return a constant reference instead of a copy to reduce the time spent inside the KUrl() constructor.
> 
> 3) Replace dynamic_cast with virtual function based upcasting inside filesForUrl() and foldersForUrl().
> 
> 
> Remaining problems:
> 
> 1) QStandardItemModel insertion is still extremely slow, but fixing that is a much larger change.
> 
> 2) There is a lot of mutex contention showing up in oprofile measumerements on a 4 core machine.  I believe this is coming from the IndexedString() constructor, but need to verify yet.  If that is the case the IndexedString() constructor either needs to be optimized or addFileSet() should switch away from IndexedString to QString or similar. 
> 
> 
> Also while creating this patch, I ran into the problem that the existing d->fileSet as used by removeFileSet() could potentially have issues when the same path is added more than once.
> 
> 
> This addresses bug 215968.
>     https://bugs.kde.org/show_bug.cgi?id=215968
> 
> 
> Diffs
> -----
> 
>   /trunk/extragear/sdk/kdevplatform/interfaces/iproject.h 1116332 
>   /trunk/extragear/sdk/kdevplatform/project/projectmodel.h 1116332 
>   /trunk/extragear/sdk/kdevplatform/project/projectmodel.cpp 1116332 
>   /trunk/extragear/sdk/kdevplatform/shell/project.h 1116332 
>   /trunk/extragear/sdk/kdevplatform/shell/project.cpp 1116332 
>   /trunk/extragear/sdk/kdevplatform/tests/testproject.h 1116332 
> 
> Diff: http://reviewboard.kde.org/r/3722/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Brandon
> 
>