<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/108914/">http://git.reviewboard.kde.org/r/108914/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 11th, 2013, 11:26 p.m. UTC, <b>Andreas Pakulat</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/108914/diff/1/?file=113723#file113723line58" style="color: black; font-weight: bold; text-decoration: underline;">outputview/outputmodel.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">58</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_timer</span><span class="o">-></span><span class="n">setInterval</span><span class="p">(</span><span class="n">m_maxProcessDelay</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Why this timer for processing? Whats wrong with blocking the event loop until a set of lines is done?</pre>
</blockquote>
<p>On February 12th, 2013, 2:19 a.m. UTC, <b>Kevin Funk</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The idea is that the cache is more likely to be filled with lines, in turn reducing the number of calls to beginInsertRow() later on.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Makes sense, just wondering wether it makes a noticable difference. In particular since most of the time stdout is being read which is usually buffered anyway.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 11th, 2013, 11:26 p.m. UTC, <b>Andreas Pakulat</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/108914/diff/1/?file=113723#file113723line59" style="color: black; font-weight: bold; text-decoration: underline;">outputview/outputmodel.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">59</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_timer</span><span class="o">-></span><span class="n">setSingleShot</span><span class="p">(</span><span class="nb">true</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This contradicts the interval-thing, please decide wether you want a single-shot timer that is restarted manually or one that runs in an interval.</pre>
</blockquote>
<p>On February 12th, 2013, 2:19 a.m. UTC, <b>Kevin Funk</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Why is that an issue? Interval sets the timeout delay of the timer, single shot defines whether it is restarted or not. They are not mutually exclusive.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Right.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 11th, 2013, 11:26 p.m. UTC, <b>Andreas Pakulat</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/108914/diff/1/?file=113723#file113723line180" style="color: black; font-weight: bold; text-decoration: underline;">outputview/outputmodel.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">177</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">parsingThread</span><span class="o">-></span><span class="n">wait</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<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 can't find anything about this in the Qt docs at the moment, but does QThread actually support "restarting" the thread? I thought I once read that this is not really supported (might work or not), but maybe my recollection is wrong.</pre>
</blockquote>
<p>On February 12th, 2013, 2:19 a.m. UTC, <b>Kevin Funk</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Seems to be fine, and worksforme. Related: http://www.qtcentre.org/threads/6548-Multiple-start()-on-same-QThread-Object.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thats not really an official statement. Would be good to verify that at least the current implementation supports this on *nix (and maybe MacOSX since KDevelop does seem to get used a bit there as well) - or find a unit-test in the qt sources that verifies this works.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 11th, 2013, 11:26 p.m. UTC, <b>Andreas Pakulat</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/108914/diff/1/?file=113723#file113723line199" style="color: black; font-weight: bold; text-decoration: underline;">outputview/outputmodel.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">196</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_sleepTimer</span><span class="p">.</span><span class="n">setInterval</span><span class="p">(</span><span class="mi">10000</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hmm, what happens if the timer times out, but there are still queued lines in the thread? i.e. the thread is currently working on some lines when its event loop is being exited. Since the event-queue is local in QThread::run() (IIRC) all the still-queued things would be lost in this case.
So I guess whats needed here is for the outputmodel to keep track of how many lines that where sent to the thread have been finished and if all are done, then start the timer to shut down the thread.</pre>
</blockquote>
<p>On February 12th, 2013, 2:19 a.m. UTC, <b>Kevin Funk</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Not an issue. ParseWorker::process() does not return to the event-loop until *all* items were processed.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thats exactly the problem. While ParseWorker::process() is working a signal can be emitted from appendLines (or rather the invokeMethod call). This will schedule a slot-invocation, but that is executed only once the event loop runs again. Now while process() is working (because there's been lots of lines) the sleeptimer times out and quits the thread. So as soon as process() returns to the event-loop it could exit itself without invoking the appendLines slot. And even if that does happen, if there is only a single line, the worker will schedule another timer which will never fire because the eventloop is going to quit.
So as I said, IMO the sleeptimer should only be started once all lines that have been scheduled for parsing have been received again inside the outputmodel as filtereditems and it should be stopped as soon as new lines come in.</pre>
<br />
<p>- Andreas</p>
<br />
<p>On February 12th, 2013, 2:24 a.m. UTC, Kevin Funk wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Kevin Funk.</div>
<p style="color: grey;"><i>Updated Feb. 12, 2013, 2:24 a.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;">Quit thread after some time
Since the OutputModel instance may live for a long time, try not to keep
a thread running that may never be used again.
Further improve the background thread
Cache the lines a certain amount of time before trying to process them.
This will guarantee that the batch is more likely in a filled state,
hence reducing the number of calls to beginInsertRow() at the end.
Also add some debug output for time measurements.
Fixup/cleanup threaded output line classification.
- use a QVector instead of a QList for efficiency,
reserve/squeeze batch sized buffer
- use a foreach loop instead of a broken for loop
- don't use shared pointers for objects that are not
shared. here we can simply rely on QObject parent-child
relation ship
- don't use a setupParser init function, use the ctor of the
OutputModelPrivate for that
- use Q_PRIVATE_SLOT to not taint our public interface
- use invokeMethod to push data to worker instead of signal
in public interface
- properly close and wait for the thread when we destroy the
OutputModelPrivate
- cleanup #include list of outputmodel.h
Mark FitleredItem as movable and make it possible to sore it in QVector
Move parsing of lines to a background thread
This moves the parsing of the added lines to a background thread to avoid
blocking the UI while the QRegEx'es are being matched. The current
implementation has a major flaw though, deleting an OutputModel causes
a crash because the thread cannot be stopped properly (at least thats what
I think is causing the crash). This might be avoidable by dropping the
manual usage of QThread and instead do this through QtConcurrent, but then
we again need manual batching with a invokeMethod to ensure that not all
lines are added at once to the view.
Removes 'removeLastLines' since the only user was the ninja-plugin and that
one should handle its special needs in its own code instead of requiring
API changes that nobody else needs.
Removes 'flushLineBuffer' since the idea of that function goes completely
against the idea of doing batches in the first place so it shouldn't exist.
If code needs to know when all items are available in the model it'll have
to connect to the model signals and verify wether the number of lines in
the model matches the expected final amount. This function is used in the
ctest-related code in kdevelop, so will need a change there too.</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>outputview/outputmodel.h <span style="color: grey">(55920beab6ee16643ef449d816d210ba343046e6)</span></li>
<li>outputview/outputmodel.cpp <span style="color: grey">(c482222c2448fe75d97911d10aaa7b89fa15bfbf)</span></li>
<li>outputview/outputexecutejob.cpp <span style="color: grey">(56167e6267d5d60caf5378bc1ea994a8a731e789)</span></li>
<li>outputview/filtereditem.h <span style="color: grey">(8eab5e9d6dbefefc203884291f6894ca2a1411a9)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108914/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>