<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/128829/">https://git.reviewboard.kde.org/r/128829/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 5th, 2016, 1:55 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;">-1</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I agree that there is an issue we need to fix, see: https://bugs.kde.org/show_bug.cgi?id=355100</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">but I disagree with the way you tackle the issue. Imo, we should try to find a way to create the parse job without holding the background parser mutex somehow. I.e. instead of fixing N parse job implementations, only fix the background parser - somehow. I'd like to investigate this tomorrow or so. Do you have any time the following days to discuss this? Or do you think it's impossible?</p></pre>
</blockquote>
<p>On September 5th, 2016, 2:04 p.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;">The clang parse job is the only one which does this, all others have relatively trivial constructors and do their intialization in run(). I'm also not sure why the DefinesAndIncludes stuff requires being foreground -- why doesn't it have its own mutex? Then this would all be simpler.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you disagree with this solution, let's discuss what we do instead -- just ping me on IRC some evening, at least after 10pm or so I'm usually there.</p></pre>
</blockquote>
<p>On September 5th, 2016, 2:56 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;">The reason I do it in the foreground is that it accesses the project model, which is not threadsafe. adding a mutex to the DAIM won't help, and making the project model threadsafe is a no-go.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the real problem imo is holding the background parser lock at all here, see the bug I referenced where we workarounded the issue in another section of our code</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;">Ah, I see.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hm, but the background parser holding the background parser mutex when creating a new job kind of makes sense. If you don't, you have to worry about things like not creating two jobs for one parse request, etc ... no?</p></pre>
<br />
<p>- Sven</p>
<br />
<p>On September 4th, 2016, 3:37 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.</div>
<div>By Sven Brauch.</div>
<p style="color: grey;"><i>Updated Sept. 4, 2016, 3:37 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;">The constructor of the parse job is called from createParseJob(), which
is called from the background parser with the background parser mutex
locked. If the constructor acquires a DUChain lock (which it did), then
any thread which acquires the DUChain lock first and then the background
parser mutex will cause a deadlock. Other language plugins do this a lot,
esp. qmljs and to some extent also python, by calling
BackgroundParser::isQueued() with the DUChain lock held.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">By doing the initialization in run() with the ForegroundLock held,
we avoid this problem.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is a severe bug, I hit it quite often and I have read more than one
report from users complaining about freezing as well.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Requires https://git.reviewboard.kde.org/r/128830/ or it will just assert out.</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;">Opened several large sessions in a debug build, didn't see any more freezes during initial parsing. Before this was really easy to observe if you opened a C++ and a JS project in parallel.</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.h <span style="color: grey">(7c0ab7a)</span></li>
<li>languages/clang/clangparsejob.cpp <span style="color: grey">(d676aea)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/settingsmanager.cpp <span style="color: grey">(590ba4a)</span></li>
<li>languages/plugins/custom-definesandincludes/definesandincludesmanager.cpp <span style="color: grey">(df1be68)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128829/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>