ForegroundLock

[Milian Wolff]

David Nolden, 07.01.2011:
> Hi, I've read your IRC discussion about the foreground lock on:
> http://webcache.googleusercontent.com/search?q=cache:gPpzdaHMDXcJ:fr.pasteb
> in.ca/2019527+QMutex+bug+foregroundlock&cd=21&hl=de&ct=clnk&gl=de
> 
> and want to do some explanations.

First up, thanks for this email, really. I appreciate it greatly. I just 
wanted to add some comments.

> - What's it's sense?

<snip>

> The second solution would be doing those actions _before_ forking into
> the background by collecting the required data beforehand. However,
> this would also mean a lot of added complexity, because right now, we
> don't know which resources exactly we will require when starting a
> parse-job for a document. It might process a random number of
> includes, it might _not require_ certain expensive informations (like
> include paths) as the document is only updated, or similar.
> 
> The third solution would be to acquire foreground privileges on-demand
> when we require a certain piece of information. We can send an event
> to the foreground process and wait for the answer in the background.
> This is the most economic approach, because it requires minimal
> changes to the source code (only local code in the place where this
> mechanism is used). However, it still requires some additional code
> for each such pattern.

Imo the first paragraph is not really noteworthy, but the last one explains 
why the ForegroundLock is neat. It made the port much easier (and might be 
used in different places in future). I still hope for the day where we rewrite 
the parse jobs though to use the other pattern you describe. And I think it is 
possible and would probably lead to a much more responsible user interface. 
KDevelop *is* noticably slower when writing, and the ForegroundLock (or the 
SmartMutex) are imo the #1 culprits for this. If we can dump parsejobs into 
the background and let them do their job completely separated from the rest 
(beside the frequent DUChain locks), I bet it will be noticably more pleasent.

Anyhow, besides this purely theoritical performance gain, there is another 
point: Maintainability. I am still, even after the lengthy discussions and 
hours spent on it, not confident to fix bugs in the lock - if there are any 
left. Otoh, the memory pattern is easy and straight forward to debug. That 
alone is for me personally a huge advantage.

> The ForegroundLock is basically simply a generalization of the third
> solution that requires nearly no local code to achieve the same thing.
> When you have the foreground-lock held, then you can safely access all
> the resources that you could access from within an event executed in
> the foreground thread, without losing your current context. The
> additional required code is minimal.
> 
> Example:
> QString editorText;
> {
>   ForegroundLock lock;
>   editorText = kateTextEditor->getText_WouldCrashWithoutForegroundLock();
> }
> 
> Of course, deadlocks may happen too, like always if you "wait" for
> another thread. However they can be prevented using a very simply
> rule: Never lock anything else before locking the foreground lock (we
> can enforce this using assertions).

I'd like to at least assert on the duchain lock. Other locks are not global 
enough to be tested there.

> - Why the ugly nested class?
> Sometimes the foreground starts nested event loops. Those are ugly,
> and the only way of dealing with them is even more uglyness. ;-) If an
> additional event-loop is started in the foreground, then the
> foreground-lock was already held beforehand. We somehow have to force
> the foreground to release it, so we can acquire it by ourselves. We
> send an event to the foreground within which the foreground releases
> the lock, waits for the background to do take it, and then re-acquires
> it.
> However, unlike what you wrote on IRC, this is not unsafe, at least
> not more "unsafe" than _any_ nested event loop. Whenever you start a
> nested event-loop anything can happen. if we simply sent an event to
> the foreground to do something, we would have the exactly the same
> problem.
> 
> The only "safety" that the foreground-lock guarantees you is, that the
> foreground thread isn't doing anything while you're holding it.

Is it really not doing *anything*? Some events will still bubble though, or?
Also, while there is already a short comment in the code, could you maybe add 
this more elaborated explanation/justification?

> - Why the custom recursion counter?
> Only this way we can unlock the foreground lock completely (in the
> nested event-loop case).
> 
> - Why the volatile?
> I added this while debugging, just to make sure that the problems
> didn't arise from some caching issues (volatile is supposed to prevent
> any kind of caching on the variable). Atomic isn't required. Anyway
> the only operation done on those variables without the lock held is
> "holderThread == QThread::currentThread()", which is impossible to get
> wrong (if it switches between true/false, _our_ thread has to do the
> transition, so it cannot be mis-informed about the outcome), so even
> the volatile shouldn't be required.

I think Atomic is required. If N threads do the check above and one of them 
changes holderThread, it might result in undefined behavior, or not? Isn't 
this exactly the kind of thing AtomicInt is supposed to be used for? Anyways, 
I'm sure Helgrind warned about this read before (without the AtomicInt).

> - Isn't it slow? We're blocking the foreground!
> Only simple and fast operations should be performed when the
> foreground lock is held. Since those operations are not thread-safe
> anyway, they must be performed in the foreground anyway, so there is
> no way of preventing these costs. However, with the ForegroundLock, we
> can still decide at a very late point of the parsing process that we
> don't need certain information, and thus we can even reduce the
> blocking of the UI. Also, we can retrieve lower amounts of information
> in separate event cycles (first the text for just one document, later
> the include-paths, etc.), which _reduces_ the blocking (in comparison
> to collecting all the info before going into background).

Right, but it's left to be seen whether a *good* approach to the memory 
pattern wouldn#t be better still. I continue to think so until the glory day 
someone implements it ;-)

> - Issues
> About the _only_ issue that I know of regarding the ForegroundLock is
> that any QObjects created with the ForegroundLock held will not be
> associated with the foreground thread. This is not relevant for 99% of
> the API calls though. Apart from this, I still haven't heard any valid
> issues, so I don't understand all the uneducated bitching.
> 
> - Milian: We need a proper rewrite!!
> There is no "proper" rewrite. The way ForegroundLock is, is about the
> only way to achieve the goal with such simplicity. If you don't like
> it, then the alternative is not changing the foreground lock, but the
> only alternative is changing the code that is using it. If you think
> that the code will become better, then you can try doing it, and then
> send the patch. However, I'm pretty sure that the code will only
> become more complicated (and maybe with more UI blocking), without a
> benefit (I couldn't think of anything simpler than the example snippet
> above). Anyway I'd be interested to see the code, so feel free trying.

I didn't mean to rewrite the ForegroundLock, I meant the whole parsing API and 
I'm perfectly aware that this is borderline madness. I won't tackle it anytime 
soon, so no worries ;-) But as you can see above I still assume the memory 
pattern would be the better approach. It works *very* well in QtCreator, I 
cannot see how it shouldn't work for us. But this is the usual "I'd think so" 
kind of argument. I have never done anything alike before so I might be 
totally wrong, who knows. I just got lots of feedback by a number of people I 
respect for their programming knowledge, all of them urging me to use the 
memory pattern. I doubt they are all wrong. And just to make sure this is not 
misunderstood: I love how you implemented this and ported KDevelop. I bet we 
would still be changing API instead of concentrating on bug fixes and 
releasing KDevelop 4.2 soon (which reminds me, I should start creating the 
tarballs for RC1). 

So let me finish it with a big thank you again. You did tremendously good 
work, and I do hope you continue to come up with such interesting and effectic 
solutions to our problems.

-- 
Milian Wolff
mail at milianw.de
http://milianw.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20110109/5f70d840/attachment.sig>