<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/123810/">https://git.reviewboard.kde.org/r/123810/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 16th, 2015, 12:04 p.m. MSK, <b>Olivier Jean de Gaalon</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;">That speedup sounds really good. I wonder if we shouldn't add some warning output if it has to fallback to findDeclaration so that we know if we're missing something that should be added to the cache. Or are there some known cases that aren't worth adding?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Caching the cache might be worth something as well... I don't have the time to look into this now, but I wonder if the symbol table could be used directly for the lookup since we already have the qid? This wouldn't find function-local declarations, but that could be handled specially.</p></pre>
</blockquote>
<p>On May 17th, 2015, 1:34 p.m. MSK, <b>Sergey Kalinichev</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;"><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 wonder if we shouldn't add some warning output if it has to fallback to findDeclaration so that we know if we're missing something that should be added to the cache. Or are there some known cases that aren't worth adding?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The latter one. There'll be about 10-50 items out of 3-5 thousand, like: NS::Enem::Enumerator when completion invoked in the function scope ( i.e. the Other context)</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;">Caching the cache might be worth something as well... I don't have the time to look into this now, but I wonder if the symbol table could be used directly for the lookup since we already have the qid? This wouldn't find function-local declarations, but that could be handled specially.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Are you suggesting to use the symbol table lookup instead of calling allDeclarations/findDeclaration? I don't think it's worth it, because e.g. for me the allDeclarations call takes about 30ms, clang code-completion about 70ms and processing all those thousands of completion items 100ms.</p></pre>
</blockquote>
<p>On May 17th, 2015, 4:56 p.m. MSK, <b>Olivier Jean de Gaalon</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;">(Is this missing a diff V2? I don't see the changes you resolved)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Does allDeclarations + populating the QHash take 30ms? Or just the call to allDeclarations? If the latter, we should maybe add a member function to build the allDeclarations hash directly.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm still curious as to how PersistentSymbolTable::getDeclarations on each qid would perform, but indeed I can't imagine it's close enough to beat the QHash building overhead.</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;"><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;">(Is this missing a diff V2? I don't see the changes you resolved)</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Oops! Uploaded it now.</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;">Does allDeclarations + populating the QHash take 30ms? </p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes. That's what I meant.</p></pre>
<br />
<p>- Sergey</p>
<br />
<p>On May 17th, 2015, 9:34 p.m. MSK, Sergey Kalinichev 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 Sergey Kalinichev.</div>
<p style="color: grey;"><i>Updated May 17, 2015, 9:34 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdev-clang
</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;">Instead of calling findDeclarations for each item found by clang, call allDeclarations once and cache the result. In case it can't find anything fall back to the findDeclarations.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This reduces code-completion time on average from 2000ms to 200ms for me.</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>codecompletion/context.cpp <span style="color: grey">(5027dcd)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123810/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>