Review Request: Enforce parse-jobs to be processed in order of their priorities

[Sven Brauch]

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

(Updated Feb. 22, 2012, 2:15 a.m.)


Review request for KDevelop, Milian Wolff and David Nolden.


Changes
-------

As discussed on IRC, I did the following changes:

* remove API changes to parseDocumens(), wasn't required anyways (I just misunderstood how the m_parseJobs array works)
* I introduced two new flags for the ParseJob class, RequiresSequentialProcessing and RespectsSequentialProcessing. By default, none of them is set. In this case, nothing changes from the old behaviour, thus leaving all language plugins which don't explicitly enable this feature unaffected (especially performance-wise). The time needed for checking those flags is in an a-few-times-per-document order of magnitude and shouldn't be relevant. Anyway, here's what they do: If RequiresSequentialProcessing is set on job A, then all jobs with a better priority than A must finish parsing before A is started -- but only if they have the second flag, RespectsSequentialProcessing, set. That way, stuff like the parsing of the current document will never starve threads, and the waiting is otherwise kept to a minimum (given you use the flags in an intelligent way, which is rather easy).
* add a new parameter to addDocument to specify the flags -- sorry, I don't see any way around this one.
* rename ownPriority to parsePriority

I also changed something else which should be reviewed -- apparently it didn't matter before but IMO it's a bug: the "break" in parseDocumentsInternal, right after "QMetaObject::invokeMethod(m_parser, "parseDocuments", Qt::QueuedConnection);" (it's backgroundparser.cpp:164 in master) only breaks out of the *inner* loop. Thus, after scheduling one document for a given priority, instead of re-executing itself and exiting, it jumps to the *next* priority and takes one document from there etc. I think that doesn't make sense, and that it must break out of both loops. Apparently that wasn't noticed so far because nobody relies on priorities and they're just used for performance optimisation (also, the less threads you have, the less effect this has), but with my changes that behaviour would definitely cause issues. Or I'm just being very stupid and don't understand what it does.


Description
-------

As recently discussed on the mailing list, it is currently unnecessarily difficult to handle the following scenario:
File A does something like "import B"; you want A to be parsed with the top-context for B available.
Various language plugins have various solutions for this problem, but none of them were considered optimal by their creators (as far as I understood).

This patch aims to adress that problem. It changes the parsejob creation algorithm by enforcing it to wait with creating a job with a worse priority as long as jobs with a better priority are still running. Example: Three documents A, B and C are scheduled for parsing, A and B with priority 0 and C with priority -1. Assuming two parse jobs are available, the old function would create two parsejobs for C and A or B (let's say A), then wait for one of them to finish, then create a third job for B. The new function will create a parsejob for C and wait until that one is finished, and then create two jobs for A and B (still simultaneously, because they have the same priority). In other words: It's guaranteed that all parse jobs running at any specific time have the same priority. (*)

Why is that useful? Because parsejob priorities can be used to adress the above problem now: Let priority(x) be the priority of the parse-job for document x. You can then parse A, and as soon as you encounter the "import B", you can schedule B with priority(A)-1 and schedule A (again) with priority(A). That's now guaranteed to first parse B and then re-parse A with the top-context for B being available.
Oh, this patch also adds a function to get a parseJobs ownPriority(), that wasn't available before.

Please tell me what you think.

Greetings,
Sven
________
(*) I'm aware of the fact that this will decrease performance by a little bit. However, I'm pretty sure it's not relevant. If the general concept of this is accepted, I'll test it.


Diffs (updated)
-----

  language/backgroundparser/backgroundparser.h 954ee17 
  language/backgroundparser/backgroundparser.cpp 7210254 
  language/backgroundparser/parsejob.h 135319c 
  language/backgroundparser/parsejob.cpp 552ef68 

Diff: http://git.reviewboard.kde.org/r/104019/diff/


Testing
-------


Thanks,

Sven Brauch

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120222/64b07a1a/attachment.html>