Opening project with real ASync way

I've implemented async way of openning project. Attatched
files
are patch. The complete ProjectController was attached as a
bunus.
because the patch file are hard to read for
ProjectController.
I excluded Job classes from patch so that we can
focus only on KDevelop::Project, and ProjectController.
I used threadweaver for job class.

It works very well. But there is one problem. Because now
ProjectBaseItems are created on separated thread, we can't
invoke
setIcon() at the constructor of ProjectBaseItem and its
subclasses.
Otherwise it segfaulted. Especially KIO::pixmapForUrl() was
the reason
of crash.

One solution would be to invoke setIcon() in the slot
connected to
KJob::done(). Starting from top item, we go recursively into
every
children and call childItem->setIcon().

Are Anyone against from this?

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

2007/5/27, dukju ahn <dukjuahngmail.com>:
> It works very well. But there is one problem. Because
now
> ProjectBaseItems are created on separated thread, we
can't invoke
> setIcon() at the constructor of ProjectBaseItem and its
subclasses.
> Otherwise it segfaulted. Especially KIO::pixmapForUrl()
was the reason
> of crash.
>
> One solution would be to invoke setIcon() in the slot
connected to
> KJob::done(). Starting from top item, we go recursively
into every
> children and call childItem->setIcon().

Here is the completed patch. The advantages are two

1. GUI doesn't hang while openning a big project

2. Project parsing became much faster because the setIcon()
in each
ProjectBaseItem isn't invoked for full recursive file/dirs.
Rather, we
just setIcon() only for toplevel items. Icon for Children
items are set up
when the user expands treeview.
I've found that setIcon() is very time-consuming operation.

Actually, Number 2 was some workaround for the problem
above.
Ironically, it brought much better performance.

If Nobody disagree, I'll commit it at monday.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

On 27.05.07 08:36:43, dukju ahn wrote:
> 2007/5/27, dukju ahn <dukjuahngmail.com>:
> >It works very well. But there is one problem.
Because now
> >ProjectBaseItems are created on separated thread,
we can't invoke
> >setIcon() at the constructor of ProjectBaseItem and
its subclasses.
> >Otherwise it segfaulted. Especially
KIO::pixmapForUrl() was the reason
> >of crash.
> >
> >One solution would be to invoke setIcon() in the
slot connected to
> >KJob::done(). Starting from top item, we go
recursively into every
> >children and call childItem->setIcon().
> 
> Here is the completed patch. The advantages are two
> 
> 1. GUI doesn't hang while openning a big project

Thats good.

> 2. Project parsing became much faster because the
setIcon() in each
> ProjectBaseItem isn't invoked for full recursive
file/dirs. Rather, we
> just setIcon() only for toplevel items. Icon for
Children items are set up
> when the user expands treeview.
> I've found that setIcon() is very time-consuming
operation.

Are you making sure to call setIcon only once already? I
mean after the
initial expansion its not needed to call setIcon again.

I'll do other comments inline in the code.

> Index: projecttreeview.cpp
>
============================================================
=======
> --- projecttreeview.cpp	(revision 668449)
> +++ projecttreeview.cpp	(working copy)
>  -54,6 +54,7 
>  
>      connect( this, SIGNAL( customContextMenuRequested(
QPoint ) ), this, SLOT( popupContextMenu( QPoint ) ) );
>      connect( this, SIGNAL( activated( QModelIndex ) ),
this, SLOT( slotActivated( QModelIndex ) ) );
> +    connect( this, SIGNAL( expanded(const
QModelIndex&) ), this, SLOT( slotExpanded( const
QModelIndex& ) ) );
>  }

I'm not sure at the moment how the projectmodel itself
works, but
eventually we can avoid having to change the view and do it
in the
model. There are virtuals that can be implemented when a
node is
expanded, in particular fetchMore().

> +void ImportProjectThread::run()
> +{
> +    if ( d->m_folder )
> +        startNextJob( d->m_folder );
> +    kDebug() << "ImportProjectThread::run()
returning" << endl;
> +}
> +
> +void
ImportProjectThread::startNextJob(ProjectFolderItem *dom)
> +{
> +    d->m_workingList <<
d->m_importer->parse(dom);
> +    while ( !d->m_workingList.isEmpty() )
> +    {
> +        ProjectFolderItem *folder =
d->m_workingList.first();
> +        d->m_workingList.pop_front();
> +
> +        startNextJob(folder);
> +    }
> +}

If I see this correctly this is a depth-first approach
right? I think a
width-first (or whatever you call that in english, where you
first run
over all items at the current level before you decend into
the next) is
better, else a user opens the projectmanagers first item and
the items
are not all there yet.  

> -void ImportProjectJob::startNextJob(ProjectFolderItem
*dom)
> +void ImportProjectJob::processList()
>  {

Hmm, is this method still needed?

> +    connect( project,
SIGNAL(importingFinished(IProject*)), this,
SLOT(projectImportingFinished(IProject*)) );

I guess I'm missing something but why can't the project
private do the
stuff in that slot directly instead of adding this
indirection, a signal
and a friend declaration? All that the project private class
needs is
asking Core::self() for the projectcontrollerinternal and
execute a
that slot with the project. Then I think the project private
class also
doesn't need a Project* but can live with a plain
IProject*.

Andreas

-- 
You love your home and want it to be beautiful.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

On May 27, 2007, at 7:36 AM, dukju ahn wrote:

> 2007/5/27, dukju ahn <dukjuahngmail.com>:
>> It works very well. But there is one problem.
Because now
>> ProjectBaseItems are created on separated thread,
we can't invoke
>> setIcon() at the constructor of ProjectBaseItem and
its subclasses.
>> Otherwise it segfaulted. Especially
KIO::pixmapForUrl() was the  
>> reason
>> of crash.
>>
>> One solution would be to invoke setIcon() in the
slot connected to
>> KJob::done(). Starting from top item, we go
recursively into every
>> children and call childItem->setIcon().
>
> Here is the completed patch. The advantages are two
>
> 1. GUI doesn't hang while openning a big project
>
> 2. Project parsing became much faster because the
setIcon() in each
> ProjectBaseItem isn't invoked for full recursive
file/dirs. Rather, we
> just setIcon() only for toplevel items. Icon for
Children items are  
> set up
> when the user expands treeview.
> I've found that setIcon() is very time-consuming
operation.
>

We still do the project parsing all at once though, right?
The reason  
for doing it all at once is that if we do it all at the
start, we  
might be able to eliminate the need to have a constant
connection  
while working on remote projects.

> Actually, Number 2 was some workaround for the problem
above.
> Ironically, it brought much better performance.
>
> If Nobody disagree, I'll commit it at monday.
> <asyncopen-managerview.patch>
> <asyncopen-project.patch>
> <asyncopen-shell.patch>

I have no disagreements, commit it now if you'd like.

General comments for next time:
  - Just make one patch file with "svn diff", it's
actually easier to  
read and review

Matt



_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

2007/5/27, Andreas Pakulat <apakugmx.de>:
> On 27.05.07 08:36:43, dukju ahn wrote:
> > 2. Project parsing became much faster because the
setIcon() in each
> > ProjectBaseItem isn't invoked for full recursive
file/dirs. Rather, we
> > just setIcon() only for toplevel items. Icon for
Children items are set up
> > when the user expands treeview.
> > I've found that setIcon() is very time-consuming
operation.
>
> Are you making sure to call setIcon only once already?
I mean after the
> initial expansion its not needed to call setIcon
again.

Yes in new implementation with fetchMore() and
canFetchMore()

> > Index: projecttreeview.cpp
> >
============================================================
=======
> > --- projecttreeview.cpp       (revision 668449)
> > +++ projecttreeview.cpp       (working copy)
> >  -54,6 +54,7 
> >
> >      connect( this, SIGNAL(
customContextMenuRequested( QPoint ) ), this, SLOT(
popupContextMenu( QPoint ) ) );
> >      connect( this, SIGNAL( activated( QModelIndex
) ), this, SLOT( slotActivated( QModelIndex ) ) );
> > +    connect( this, SIGNAL( expanded(const
QModelIndex&) ), this, SLOT( slotExpanded( const
QModelIndex& ) ) );
> >  }
>
> I'm not sure at the moment how the projectmodel itself
works, but
> eventually we can avoid having to change the view and
do it in the
> model. There are virtuals that can be implemented when
a node is
> expanded, in particular fetchMore().

Yes. fetchMore() worked very well.

> If I see this correctly this is a depth-first approach
right? I think a
> width-first (or whatever you call that in english,
where you first run
> over all items at the current level before you decend
into the next) is
> better, else a user opens the projectmanagers first
item and the items
> are not all there yet.

During the process of parsing, there should be no GUI
changing
because QWidgets are not thread-safe. Only when every
parsing is
completed, the user can see items. So Don't worry about the
empty
items of first parent.

> > -void
ImportProjectJob::startNextJob(ProjectFolderItem *dom)
> > +void ImportProjectJob::processList()
> >  {
>
> Hmm, is this method still needed?

I was unsure about this method. I'll remove it

> > +    connect( project,
SIGNAL(importingFinished(IProject*)), this,
SLOT(projectImportingFinished(IProject*)) );
>
> I guess I'm missing something but why can't the project
private do the
> stuff in that slot directly instead of adding this
indirection, a signal
> and a friend declaration? All that the project private
class needs is
> asking Core::self() for the projectcontrollerinternal
and execute a
> that slot with the project.
> Then I think the project private class also
> doesn't need a Project* but can live with a plain
IProject*.

Unfortunately, Core::self() returns _I_ProjectController,
not ProjectController.
If we follow your idea, this projectImportingFinished() slot
should be
added into IProjectController also.
Also, It is not natural design to invoke ProjectController's
method
inside each Project.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

On 28.05.07 02:22:59, dukju ahn wrote:
> 2007/5/27, Andreas Pakulat <apakugmx.de>:
> > On 27.05.07 08:36:43, dukju ahn wrote:
> > If I see this correctly this is a depth-first
approach right? I think a
> > width-first (or whatever you call that in english,
where you first run
> > over all items at the current level before you
decend into the next) is
> > better, else a user opens the projectmanagers
first item and the items
> > are not all there yet.
> 
> During the process of parsing, there should be no GUI
changing
> because QWidgets are not thread-safe. Only when every
parsing is
> completed, the user can see items. So Don't worry about
the empty
> items of first parent.

So you're saying that until the whole project is parsed the
projectmanagerview shows nothing? Thats not good, IMHO the
projectmanager view should be populated with items as soon
as the
manager plugin created them. btw, are the qstandarditems
created in the
non-gui thread and then transferred into the gui thread to
be used in
the GUI or are they created inside the GUI thread? What
about
concurrency? the parse() function of the manager isn't
needed to be
threadsafe and in fact I doubt it is at the moment. For
example at some
point I'll need to access at least the projects kconfig
object in qmake
manager.

Sorry I didn't think about this yesterday, but it was a bit
late here.

> > > +    connect( project,
SIGNAL(importingFinished(IProject*)), this,
SLOT(projectImportingFinished(IProject*)) );
> >
> > I guess I'm missing something but why can't the
project private do the
> > stuff in that slot directly instead of adding this
indirection, a signal
> > and a friend declaration? All that the project
private class needs is
> > asking Core::self() for the
projectcontrollerinternal and execute a
> > that slot with the project.
> > Then I think the project private class also
> > doesn't need a Project* but can live with a plain
IProject*.
> 
> Unfortunately, Core::self() returns
_I_ProjectController, not ProjectController.

Right, thats why Core has projectControllerInternal().

> If we follow your idea, this projectImportingFinished()
slot should be
> added into IProjectController also.
> Also, It is not natural design to invoke
ProjectController's method
> inside each Project.

Why is that not natural design? Currently I see a signal and
a slot
added for 1 call, which can be done directly anyway.
Signal/Slot should
be used if you want to notify multiple objects of something
or if you
expect to connect multiple signals to a slot, or both -
IMHO.

Andreas

-- 
Expect a letter from a friend who will ask a favor of you.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

2007/5/28, Andreas Pakulat <apakugmx.de>:
> On 28.05.07 02:22:59, dukju ahn wrote:
> > During the process of parsing, there should be no
GUI changing
> > because QWidgets are not thread-safe. Only when
every parsing is
> > completed, the user can see items. So Don't worry
about the empty
> > items of first parent.
>
> So you're saying that until the whole project is parsed
the
> projectmanagerview shows nothing?

Yes.

> Thats not good, IMHO the
> projectmanager view should be populated with items as
soon as the
> manager plugin created them.

But why we should let the user see project contents even
when
the project is not fully parsed? It only confuses user
anyway.

> btw, are the qstandarditems created in the
> non-gui thread and then transferred into the gui thread
to be used in
> the GUI or are they created inside the GUI thread?

The top ProjectItem is created in GUI, and his children are
created
in separate thread. ImportJob takes ProjectItem as its
argument.

> What about
> concurrency? the parse() function of the manager isn't
needed to be
> threadsafe and in fact I doubt it is at the moment. For
example at some
> point I'll need to access at least the projects kconfig
object in qmake
> manager.

Aah, real big problem. But see KSharedConfig section at
KConfig
techbase tutorial below.--

It thus provides a way to reference the same configuration
object from
multiple places in your application without the extra
overhead of
separate objects or concerns about syncronizing writes to
disk even if
the configuration object is updated from multiple code
paths.

But I'm not sure whether this means thread-safe.
Rather than KConfig, we usually don't modify any member
variable
in parse(). So its thread-safe.

Or I think we should force parse() to be thread-safe if it
is necessary.
I can't imagine any other way than using thread to make
async.

> Sorry I didn't think about this yesterday, but it was a
bit late here.

I didn't think about that too. Don't need to be sorry 

> > Unfortunately, Core::self() returns
_I_ProjectController, not ProjectController.
>
> Right, thats why Core has projectControllerInternal().

No. Core doesn't have projectControllerInernal. Rather it
has
uiControllerInternal(), docCtrlInternal(), and
pluginCtrlInternal().

> > If we follow your idea, this
projectImportingFinished() slot should be
> > added into IProjectController also.
> > Also, It is not natural design to invoke
ProjectController's method
> > inside each Project.
>
> Why is that not natural design? Currently I see a
signal and a slot
> added for 1 call, which can be done directly anyway.
Signal/Slot should
> be used if you want to notify multiple objects of
something or if you
> expect to connect multiple signals to a slot, or both -
IMHO.

Well ok. You are right. I'll change it.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

On Monday 28 May 2007 07:55, dukju ahn wrote:
> 2007/5/28, Andreas Pakulat <apakugmx.de>:
> > On 28.05.07 02:22:59, dukju ahn wrote:
> > > During the process of parsing, there should
be no GUI changing
> > > because QWidgets are not thread-safe. Only
when every parsing is
> > > completed, the user can see items. So Don't
worry about the empty
> > > items of first parent.
> >
> > So you're saying that until the whole project is
parsed the
> > projectmanagerview shows nothing?
>
> Yes.
>

That's worse than blocking the GUI, IMO

> > Thats not good, IMHO the
> > projectmanager view should be populated with items
as soon as the
> > manager plugin created them.
>
> But why we should let the user see project contents
even when
> the project is not fully parsed? It only confuses user
anyway.
>

Then we need to provide a progress indicator so that the
user knows we're 
still working on parsing her project.

> > btw, are the qstandarditems created in the
> > non-gui thread and then transferred into the gui
thread to be used in
> > the GUI or are they created inside the GUI
thread?
>
> The top ProjectItem is created in GUI, and his children
are created
> in separate thread. ImportJob takes ProjectItem as its
argument.
>
> > What about
> > concurrency? the parse() function of the manager
isn't needed to be
> > threadsafe and in fact I doubt it is at the
moment. For example at some
> > point I'll need to access at least the projects
kconfig object in qmake
> > manager.
>
> Aah, real big problem. But see KSharedConfig section at
KConfig
> techbase tutorial below.--
>
> It thus provides a way to reference the same
configuration object from
> multiple places in your application without the extra
overhead of
> separate objects or concerns about syncronizing writes
to disk even if
> the configuration object is updated from multiple code
paths.
>
> But I'm not sure whether this means thread-safe.
> Rather than KConfig, we usually don't modify any member
variable
> in parse(). So its thread-safe.
>
> Or I think we should force parse() to be thread-safe if
it is necessary.
> I can't imagine any other way than using thread to make
async.
>

In general, as long as we're only reading from it, we might
be able to get 
away with it. But it's better just to do proper locking in
the first place.

> > Sorry I didn't think about this yesterday, but it
was a bit late here.
>
> I didn't think about that too. Don't need to be sorry

>
> > > Unfortunately, Core::self() returns
_I_ProjectController, not
> > > ProjectController.
> >
> > Right, thats why Core has
projectControllerInternal().
>
> No. Core doesn't have projectControllerInernal. Rather
it has
> uiControllerInternal(), docCtrlInternal(), and
pluginCtrlInternal().
>
> > > If we follow your idea, this
projectImportingFinished() slot should be
> > > added into IProjectController also.
> > > Also, It is not natural design to invoke
ProjectController's method
> > > inside each Project.
> >
> > Why is that not natural design? Currently I see a
signal and a slot
> > added for 1 call, which can be done directly
anyway. Signal/Slot should
> > be used if you want to notify multiple objects of
something or if you
> > expect to connect multiple signals to a slot, or
both - IMHO.
>
> Well ok. You are right. I'll change it.
>
-- 
Matt

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

2007/5/28, Matt Rogers <mattrkde.org>:
> On Monday 28 May 2007 07:55, dukju ahn wrote:
> > 2007/5/28, Andreas Pakulat <apakugmx.de>:
> > >
> > > So you're saying that until the whole project
is parsed the
> > > projectmanagerview shows nothing?
> >
> > Yes.
> >
> That's worse than blocking the GUI, IMO

Why is that worse? I guess you misunderstand. If there are
loaded projects
already, and we are parsing some new projects, then the
projectmanager
view still shows already loaded project. The word
"shows nothing" means
that currently parsing project are not shown. Already loaded
project
can be shown as usual.

> > > Thats not good, IMHO the
> > > projectmanager view should be populated with
items as soon as the
> > > manager plugin created them.
> >
> > But why we should let the user see project
contents even when
> > the project is not fully parsed? It only confuses
user anyway.
> >
>
> Then we need to provide a progress indicator so that
the user knows we're
> still working on parsing her project.

That's a good idea. ImportJob is KJob so its easy.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

On 28.05.07 08:55:09, dukju ahn wrote:
> 2007/5/28, Andreas Pakulat <apakugmx.de>:
> > On 28.05.07 02:22:59, dukju ahn wrote:
> > Thats not good, IMHO the
> > projectmanager view should be populated with items
as soon as the
> > manager plugin created them.
> 
> But why we should let the user see project contents
even when
> the project is not fully parsed? It only confuses user
anyway.

No, seeing the project populate tells the user that kdevelop
actually
reads it currently. While doing nothing for half a minute
(ui-wise)
tells the user it silently failed, which is bad. IMHO.

> > btw, are the qstandarditems created in the
> > non-gui thread and then transferred into the gui
thread to be used in
> > the GUI or are they created inside the GUI
thread?
> 
> The top ProjectItem is created in GUI, and his children
are created
> in separate thread. ImportJob takes ProjectItem as its
argument.

So will connections done during creation of the items work
after the
thread has died? I mean if the items are created in a
separate thread
then  the connections are all queuedconnections by default
(the manager
plugin lives in the gui thread) and this (AFAIK) doesn't
change if the
thread dies.

> > What about
> > concurrency? the parse() function of the manager
isn't needed to be
> > threadsafe and in fact I doubt it is at the
moment. For example at some
> > point I'll need to access at least the projects
kconfig object in qmake
> > manager.
> 
> Aah, real big problem. But see KSharedConfig section at
KConfig
> techbase tutorial below.--

KSharedConfig makes sure you never get more than 1 KConfig
object in
your application because it adds implicit sharing of the
objects (like
QString class). And IIRC the shared classes of Qt are not
thread-safe
due to this mechanism.

> But I'm not sure whether this means thread-safe.
> Rather than KConfig, we usually don't modify any member
variable
> in parse(). So its thread-safe.

Well, we should use proper locking to access stuff from
various threads,
no matter wether we read or write.

> Or I think we should force parse() to be thread-safe if
it is necessary.
> I can't imagine any other way than using thread to make
async.

Its easy to do it different: use fetchMore/canFetchMore to
let the model
do the parsing when the project tree is traversed. So in
fetchMore the
model would ask the projectmanger to parse the parent item
and the
manager can check wether it already parsed this one or not
(by setting a
flag or similar, or we can do this in the ProjectBaseItem as
wel).

> > > Unfortunately, Core::self() returns
_I_ProjectController, not ProjectController.
> >
> > Right, thats why Core has
projectControllerInternal().
> 
> No. Core doesn't have projectControllerInernal. Rather
it has
> uiControllerInternal(), docCtrlInternal(), and
pluginCtrlInternal().

Then add it please 

Andreas

-- 
Never look up when dragons fly overhead.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel