<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/104019/">http://git.reviewboard.kde.org/r/104019/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't like the way this idea is implemented. The api is clutterred with implementation-specific changes that should be hidden (e.g. via Q_PRIVATE_SLOT or similar). the "own priority" could be stored in a QMap<int, ParseJob*> that would allow fast lookup of the "best priority" via map.begin().key() (lowest value == highest priority).
Aside from that I don't like the fact that we will end up with cases where our parse threads are not doing *anything* just because some document is waiting for a reparse. Is there really no other way to solve this issue?
And at the *very* least I'd like to see some benchmarks (before/after) and would probably have to profile it on my own using vtune or similar to figure out the impact of this approach. But I'm pretty sure that the linear search you have there is killing performance.</pre>
<br />
<p>- Milian</p>
<br />
<p>On February 19th, 2012, 2:40 p.m., Sven Brauch wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop, Milian Wolff and David Nolden.</div>
<div>By Sven Brauch.</div>
<p style="color: grey;"><i>Updated Feb. 19, 2012, 2:40 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>language/backgroundparser/backgroundparser.h <span style="color: grey">(954ee17)</span></li>
<li>language/backgroundparser/backgroundparser.cpp <span style="color: grey">(7210254)</span></li>
<li>language/backgroundparser/parsejob.h <span style="color: grey">(135319c)</span></li>
<li>language/backgroundparser/parsejob.cpp <span style="color: grey">(552ef68)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104019/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>