[Panel-devel] Multithreaded krunner

[Aaron J. Seigo]

On Friday 23 November 2007, Ryan Bitanga wrote:
> > would take some experimenting i suppose.
>
> What I don't like about this solution is authors of runners have to
> know where to place the checks. The code won't look as nice and I'd be
> more comfortable with them only focusing on what the runner needs to
> be done. But this should perform better than the other solution
> because CPU time won't be wasted for unneeded matches.

agreed; and i don't think the potential cpu savings are worth it.

> > and bring it down to NumberOfRunners. this would also allow us to get rid
> > of the locks in most of the SearchContext methods, only having to make
> > the copy constructor thread safe. that should help with the locking, no?
>
> Yes, this greatly reduce the penalties incurred by using locks. Only
> the call(s) to add(*)Matches would need to be thread-safe. I'm not
> sure if the constructor would need to be made thread-safe. Either a
> local mutex in performMatch or a copy constructor would be sufficient.
> The problem is SearchContext is a QObject, and QObjects can't be
> children of objects in a different thread. We could create another

the reason SearchContext is a QObject was to manage the ownership of the 
QActions. so the options are two fold: either make SearchContext not a 
QObject and move to the data-only SearchMatch approach, or do as you note 
here and make some special constructors. i think it would be nice to keep 
SearchContext a QObject if only because that leaves the door open to 
signal/slot usage later. this might be useful if we move the default action 
selection into SearchContext as it could then signal when the default 
changes.

so .. yeah, new constructor it is. =)

> constructor, say SearchContext( QObject* parent, const SearchContext&
> context) to get around this. The new constructor will have to be
> thread-safe.
>
> Then  AbstractRunner::performMatch(SearchContext &context)
>  {
>      SearchContext localContext(context);
>      match(localContext);
> ...
> }
> would become
> AbstractRunner::performMatch(SearchContext &context)
> {
>     SearchContext localContext = new SearchContext( 0, context);
>     match(localContext);
> ...
>    delete localContext;
> }

well, i'd keep SearchContext on the stack (the delete is in the same scope as 
the new) ... and there's also no real reason SearchContext needs to be a 
QObject.

> The only drawback I see with this is that if it takes the runner a
> long time between finding matches, krunner will seem to be less
> responsive. But I can't really think of an example where a runner take
> a noticeable amount of time between finding matches so it's probably
> worth the trade-off.

agreed. especially since the only runners that really, really matter here 
would be the ones that are system critical, such as the shell command runner 
which run very quickly.

> I'll see if I can write patches for both solutions then post them here
> for comments.

why don't we just try one approach, picking the one that seems best. which 
would be the SearchContext copy passed in to the match, discarding 
mismatches.

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Trolltech
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/panel-devel/attachments/20071123/0ef5b983/attachment.pgp