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

[Brandon Ehle]

> On 2010-04-21 17:04:52, Milian Wolff wrote:
> > 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

re 1: Yeah, I verifed that at least the cmake project loader inserts the same URL twice with different items.  When I noticed that before I switched from the QHash I was using previously to the QMultiHash that is currently in use.

re 2: I latched on to the existing calls because of my limited knowledge of kdevplatform; with the current code hooks, if the existing addToFileSet() code is buggy, then the fileHash will also be buggy.  I'm not really familiar enough with QT and Kde to make a change to using the signal handling and I'm not sure what the performance implications are without going through the documentation first.

I've been doing oprofile captures rather than callgrind because the original code takes an extremely long time to run.  I'm not sure how much the full time is, but I have let it run for more than 4 hours and it wasn't 25% complete yet in the progress bar.  I did use callgrind_control to capture a couple of snapshots to zero on the problem code, but you cannot really quantify if things are better or worse because the captures contains an arbitrary amount of time.

If there isn't already code somewhere to do this already, I will add some code to time the entire project import process and display it in the IDE as even oprofile slows down the process significantly.


> On 2010-04-21 17:04:52, Milian Wolff wrote:
> > /trunk/extragear/sdk/kdevplatform/shell/project.cpp, line 675
> > <http://reviewboard.kde.org/r/3722/diff/1/?file=24179#file24179line675>
> >
> >     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.

I believe the fileSet handling is actually buggy in this case given the callers.  If one reference to a URL gets deleted, then the URL is completely removed from the fileSet even though there may still be references to it.  I'm not sure whether this can actually be triggered in practice, but yes the fileSet and the fileHash purposely have different behavior in this regard. 

void ProjectFileItem::setUrl( const KUrl& url )
{
    Q_D(ProjectFileItem);
    if(!d->m_url.isEmpty())
        project()->removeFromFileSet( KDevelop::IndexedString(d->m_url), this );
    Q_ASSERT(!url.isEmpty());
    project()->addToFileSet( KDevelop::IndexedString(url), this );
    
    d->m_url = url;
    d->m_fileName = d->m_url.fileName();
    setText( d->m_fileName );
    setToolTip( text() );
    setIcon(KIcon(KMimeType::findByUrl(url, 0, false, true)->iconName(url)));
}

ProjectFileItem::~ProjectFileItem()
{
    Q_D(ProjectFileItem);
    project()->removeFromFileSet(KDevelop::IndexedString(d->m_url), this);
}


- Brandon


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


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