> On Sept. 6, 2012, 12:29 p.m., David Faure wrote: > > A number of comments on the implementation, but also a more general > > comment: does it even make sense to use a Speller in multiple threads, and > > to change the language from one thread while another one is using the > > speller for spell-checking? It sounds like, even if a mutex/lock can > > prevent crashes, this is a weird thing to do anyway, since you have no idea > > at which point the spell-checking will switch to another language... could > > happen in the very middle of a sentence... > > > > Maybe it would make more sense to "post" the language change operation to > > the spell-checking thread using the same mechanism as the one used to > > "post" spellchecking requests to it? (Disclaimer: I know nothing of the > > krunner architecture). > > Simeon Bird wrote: > Thanks for the review. > > I agree that this is a slightly weird thing to do, but, as you surmise, > it was the only way I could think of to make the feature work properly. > > As far as I understand it (from reading the documentation), the way > krunner works is to call Plasma::AbstractRunner::match in a new thread for > every runner every time input is entered. > > So if I enter: > > "spell hello" > > I will be called with 's' 'sp' 'spe'...and so on until I get the whole > match, without waiting for the previous threads to return. Thus match has to > be thread-safe. > > The feature I'm trying to fix here is to be able to enter "spell french > bonjour" and have it spell something. > The runner is responsible for parsing the input and checking whether it > should do anything else. > So we don't actually post spellchecking requests, we just post some > input, and detect that we should spell-check, and change the language, by > parsing strings. So I couldn't see a way to do this except to call > Sonnet::Speller::setLanguage() in Plasma::AbstractRunner::match, > which meant that for it to work I had to make match thread-safe. > > Sorry. I'm open to suggestions if you have a better idea. > > I've fixed your other comments in v2 of the diff, let me know if there is > anything else. > > Thomas Lübking wrote: > would it not make more sense to just kill the pending (and now > worthless?) thread instead (no idea how expensive this spellchecking is to be > threaded) to not waste cpu on a result that will be ignored anyway? > > David Faure wrote: > Forcefully killing threads (QThread::terminate()) is a big no-no, it > leaks memory, potentially worse: it can leave a mutex locked, so any further > thread trying to lock it, will hang for ever. > > The only way to terminate a thread is to ask it nicely to terminate from > within (i.e. to exit run()), which means it has to finish what it's currently > doing, first, before it looks up for the next command or posted event which > tells it to quit. > > Thomas Lübking wrote: > That is generally true and i admit to not know how the plasma spell > checker is implemented (or even "where" ;-), but would expect some sort of ro > non-joinable threading, operating on shared data (as the only purpose seems > to prevent blocking the main event loop - not running two checks at the same > time and merge their results) ... or does QThread alone bring structures to > forbid such approach (The API doc does not mention inevitable leaks for > QThread itself on ::terminate())? > > (PS: I just start to wonder whether the speller threads are then tagged > to catch 2,1 exits...) > > Simeon Bird wrote: > I don't entirely follow what you mean in the last comment; what do you > mean 'tagged to catch 2,1 exits'? > > The spellchecking runner is, I think, very cheap compared to some of the > other runners (eg, the nepomuk search), all of which are called at once. > > But, be that as it may, the spell-check has no control over the threading > model of krunner, which it just inherits from Plasma::AbstractRunner, and > changing that model for everything seems a bit overkill for this, even if we > understood fully how it works (I certainly don't; I just took the > documentation at face value, which may or may not be a good idea). > > A reasonable alternative, if you don't like doing the commit in principle > (the mutex might well slow down the spell-check), is to just remove the > problematic feature (which has never worked anyway). If krunner only > spell-checked words in the default dictionary, we wouldn't need to call > setLanguage(), and everything would be fine, as it was before 2010. > > Thomas Lübking wrote: > Thread execution is not predictable so in general it's possible that you > start a thread (1), then start another thread (2) and thread 2 finishes > before thread 1 > Since i assume that in this case the last exiting thread will determine > the sanity of the token, this would eg. mean that if you spellcheck "threa" > and then "thread" but "threa" finishes last, the token "thread" would be > determined misspelled. > > A common approach to handle this is to "tag" (basically with an > increment) the threads/results so that if you get a result from thread "1" > and already got a result from thread "2" before, you know that the result > from thread "1" is already irrelevant and simply drop it instead of polluting > your result. > > I have no particular opinion on this patch or the krunner spell checker > in general (except assuming that threading it will likely be overhead anyway > - esp. if one sequentially navigated through a tree structure - and given the > type rate a normal human being can achieve ;-)
Ah, I see. This is getting somewhat educational for me :) - Simeon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106242/#review18623 ----------------------------------------------------------- On Sept. 9, 2012, 10:06 p.m., Simeon Bird wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106242/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2012, 10:06 p.m.) > > > Review request for kdelibs and Plasma. > > > Description > ------- > > Krunner's spellcheck plugin has been pretty broken since > bd291d21f096a714a171e7af3a534ba345ca5659 (about two years ago) because it > called Sonnet::Speller::setLanguage every time the spellchecker was invoked, > which was (very much) not thread-safe. > > This patch makes Sonnet::Speller::setLanguage threadsafe by protecting all > access to the internal dict pointer using QReadWriteLock. > > A related review request is 106244, which adds more fixes to the spellcheck > feature. > > > This addresses bugs 264779 and 303831. > http://bugs.kde.org/show_bug.cgi?id=264779 > http://bugs.kde.org/show_bug.cgi?id=303831 > > > Diffs > ----- > > kdecore/sonnet/speller.cpp b19e74d > > Diff: http://git.reviewboard.kde.org/r/106242/diff/ > > > Testing > ------- > > Compiled, installed, used for a week or so, spellchecked a bunch of things. > > > Thanks, > > Simeon Bird > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
