<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/128480/">https://git.reviewboard.kde.org/r/128480/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 20th, 2016, 9:55 a.m. UTC, <b>David Nolden</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Copy of my e-mail: While reviewing the clang duchain code, I found some severe problems which are probably the main reason for these glitches to appear.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The core of the problem is, that the code assumes that for "unmodifed" files, it doesn't need to do any revision mapping, which is wrong. When you save a file in revision 10, the revision will still be 10 after saving, and the mapping still needs to be performed in the same way.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Some of these things were done on-demand with the foreground lock in the old cpp plugin, but that isn't possible straightforwardly any more, because the "unsaved files" need to be prepared <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">before</em> parsing now.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So in principle, what needs to be done is:
* Before parsing, either in foreground or with the foreground lock, the text of <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">all</em> open documents need to be read, and the exact corresponding document revisions need to be locked and stored in the parse session. When the extracted text is used for parsing, then this can be used for glitch-free highlighting and navigation.</p></pre>
</blockquote>
<p>On July 20th, 2016, 10:05 a.m. UTC, <b>Sven Brauch</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks David, I think I agree with your conclusion. The root of all evil is the "!isModified()" shortcut (I already mentioned it as odd above but it slowly becomes clear to me just how much trouble it can cause). Should we just remove that ...? It seems potentially slow when you have hundreds of files open.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">While we're at it, David, what do you think about storing the revision references in the top context instead of the change tracker? I still think we sometimes have the case that this happens: parse job runs -> finishes -> new job starts, resets tracker and discards revision -> highlighter is invoked for old job. We somehow have to avoid this.</p></pre>
</blockquote>
<p>On July 20th, 2016, 10:45 a.m. UTC, <b>David Nolden</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Some cache would be needed to avoid the overhead of re-reading all the files. Maybe a global QVector<UnsavedFile> should be managed and kept up-to-date in the foreground, and just copied away with each parse-job. Due to the copy-on-write property of Qt containers, this should be very efficient. Each UnsavedFile should also have an own RevisionReference then, to check whether it's up-to-date etc.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In principle, RevisionLockerAndClearer is safe w.r.t. it being deleted from within a background thread, so it probably would make sense to attach such a locker to each top-context. In practice though, I'm not sure whether this could actually cause the glitches, because, I think, mapping even works with revisions that are <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">not</em> locked. Nevertheless, this case needs to be considered of course.</p></pre>
</blockquote>
<p>On July 20th, 2016, 11:10 a.m. UTC, <b>Sven Brauch</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks for your advice, I'll have a look.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think, mapping even works with revisions that are not locked</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Pretty sure that is not the case, in the document change tracker there is this code:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">if((fromRevision == -1 || holdingRevision(fromRevision)) && (toRevision == -1 || holdingRevision(toRevision))) { ... }
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It explicitly checks whether the revision is locked and does nothing otherwise.</p></pre>
</blockquote>
<p>On July 20th, 2016, 11:21 a.m. UTC, <b>Milian Wolff</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Removing the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">!isModified()</code> shortcut is a very bad idea, performance wise. Please let us find the culprit and fix that instead of removing legitimate features that are implemented buggily.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As I wrote in my email in response to David's: Can you please try out to lock the revision in the UnsavedFile? Maybe that's all that's missing!</p></pre>
</blockquote>
<p>On July 20th, 2016, 11:23 a.m. UTC, <b>Sven Brauch</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But David is right, the isModified() is just wrong -- it does not mean <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">anything</em> whether the file is modified or not :/
Just imagine I press save after each keypress, isModified() will always be false but otherwise the exact same thing needs to happen.</p></pre>
</blockquote>
<p>On July 20th, 2016, 11:33 a.m. UTC, <b>David Nolden</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Milian: I didn't see any email.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Anyway, this probably <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">is</em> the real culprit and you cannot work around it. We just have to implement a solution which is efficient performance-wise. I think my previous proposal with a global QVector<UnsavedFile> could actually be more efficient than what's implemented now, because it could cache the extraction of the contents.</p></pre>
</blockquote>
<p>On July 20th, 2016, 7:57 p.m. UTC, <b>Milian Wolff</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Oooh! Now I understand what you mean, <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">that</em> <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">isModified()</code>! I really should have studied the code again. I was assuming you where talking about <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">isUpdateRequired()</code> in the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">::run()</code> method. Sure, if removing <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">isModified()</code> helps (and now I also understand how it would help), feel free to remove it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And I agree with David that introducing a global cache could help, but so far I haven't seen this taking a considerable amount of time either, so it's not worth doing it right now, I think. I.e. just remove it, and profile a normal duchainify run. If you get less than 0.1% samples there, then it shouldn't be too bad. Memory wise it it's a bigger impact, esp. if you have hundreds of files open.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That last part is btw. why I added that shortcut. We often have tons of docs open, but only a few of them we work on and thus have unsaved changes. Feel free to remove the check for now as a hotfix (if it really fixes the issue). But thinking ahead, is there no way we could keep the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">isModified()</code> check in place and lock the associated revision and use that one? Actually, if we'd have a global cache as suggested by David, we could in principle have that one updated in a thread safe manner from the foreground and then use it as needed in the background thread when we hand over to clang, with the revisions from that version, or?</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Milian: Since it would happen in the constructor of the parse job -- which is always in the foreground -- the cache management would be thread safe by definition.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Sven: Actually I think it would be best to put this into ParsingEnvironmentFile, because this is where the revision management is living already now. So you could simply add a buddy function called "setRevisionReference" which takes the locked revision next to ParsingEnvironmentFile::setModificationRevision, or alternatively do this in ClangParsingEnvironment if the API shouldn't be changed.</p></pre>
<br />
<p>- David</p>
<br />
<p>On July 20th, 2016, 10:36 p.m. UTC, Sven Brauch wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDevelop, Kevin Funk and Milian Wolff.</div>
<div>By Sven Brauch.</div>
<p style="color: grey;"><i>Updated July 20, 2016, 10:36 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevelop
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Terrible patch, I know. Sorry. It cost a lot of nerves and time though and I want somebody to look at / try out whether this is a working fix in principle.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In the end, the issue(s) are simple:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">we need to hold the foreground lock between reading the modification revision and reading the contents. Otherwise they can mismatch.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">in buildDUChain(), there was</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">envFile->setModificationRevision(ModificationRevision::revisionForFile(context->url()));</p>
</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">which is wrong. It sets the modification revision of the parsing environment file to the revision the file has <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">right now</em>. The contents, however, were read before in the constructor, and several milliseconds might have passed until this line is reached, with no locks held to prevent anything.
Instead, we have to set the revision to the one which was stored when the tracker was last reset (which happened right after reading the contents, coupled to the contents by the foreground lock).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Actually I'm not sure if the foreground lock is required in this case, because clang for some reason reads the contents in the constructor, which afaik runs in the main thread anyways.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Very hard to reproduce. Apply this patch to kdevplatform:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #000080; font-weight: bold">diff --git a/language/backgroundparser/documentchangetracker.cpp b/language/backgroundparser/documentchangetracker.cpp</span>
<span style="color: #000080; font-weight: bold">index 294bc14..6f56250 100644</span>
<span style="color: #A00000">--- a/language/backgroundparser/documentchangetracker.cpp</span>
<span style="color: #00A000">+++ b/language/backgroundparser/documentchangetracker.cpp</span>
<span style="color: #800080; font-weight: bold">@@ -237,6 +237,11 @@ KDevelop::RangeInRevision DocumentChangeTracker::transformBetweenRevisions(KDeve</span>
{
VERIFY_FOREGROUND_LOCKED
<span style="color: #00A000">+ if ( !((fromRevision == -1 || holdingRevision(fromRevision)) && (toRevision == -1 || holdingRevision(toRevision)) ) ) {</span>
<span style="color: #00A000">+ qWarning() << "invalid transform: from" << fromRevision << "to" << toRevision</span>
<span style="color: #00A000">+ << "but not both revisions held: [from/to]:" << holdingRevision(fromRevision) << holdingRevision(toRevision);</span>
<span style="color: #00A000">+ };</span>
<span style="color: #00A000">+</span>
if((fromRevision == -1 || holdingRevision(fromRevision)) && (toRevision == -1 || holdingRevision(toRevision)))
{
m_moving->transformCursor(range.start.line, range.start.column, KTextEditor::MovingCursor::MoveOnInsert, fromRevision, toRevision);
<span style="color: #800080; font-weight: bold">@@ -348,6 +353,8 @@ void DocumentChangeTracker::unlockRevision(qint64 revision)</span>
m_moving->unlockRevision(revision);
m_revisionLocks.erase(it);
}
<span style="color: #00A000">+</span>
<span style="color: #00A000">+ qDebug() << "** clearing revision" << revision;</span>
}
qint64 RevisionLockerAndClearer::revision() const
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You get <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">lots</em> of those warnings iff you manage to trigger the issue. The best bet seems to be open a relatively large cpp file, and keep removing text and readding it with Ctrl+Z with varying wait times in between (500ms-parse-delay-ish). Do that for a minute or two and it will trigger eventually.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You still get a few of the warning messages sometimes even with the patch applied (but far more without). I think they are from the problem reporter plugin, and I'm not sure if the plugin does something wrong or the parse job.</p></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>languages/clang/clangparsejob.cpp <span style="color: grey">(8375eb5)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128480/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>