clang's CXTranslationUnit_PrecompiledPreamble madness

[Kevin Funk]

On Saturday, January 09, 2016 06:28:02 PM Milian Wolff wrote:
> Hey all, esp. Sergey, Kevin and Olivier.
> 
> After quite some time I finally sat down today to write a proper benchmark
> for our clang-based code completion. It's relatively slow for big C++
> include chains as we all know. It's still bearable most of the time where
> it's in the ~250ms space on my machine, but definitely noticeable and of
> course worse on slower machines.
> 
> Now during the recent Qt World Summit I chatted with Marco Bubke who's
> nowadays working on Qt Creator's clang code model. One nugget he shared with
> me is that Clang's C API won't actually create a preamble if you don't call
> clang_reparseTranslationUnit after the initial clang_parseTranslationUnit2
> call...
> 
> With my new benchmark you can see the impact this has, on a KDev* build
> without any optimizations:
> 
> no preamble:
> 
> RESULT : BenchCodeCompletion::benchCodeCompletion():"empty":
>      2.4 msecs per iteration (total: 79, iterations: 32)
> --
> RESULT : BenchCodeCompletion::benchCodeCompletion():"stl":
>      171 msecs per iteration (total: 171, iterations: 1)
> --
> RESULT : BenchCodeCompletion::benchCodeCompletion():"clib":
>      66 msecs per iteration (total: 66, iterations: 1)
> 
> with preamble:
> 
> RESULT : BenchCodeCompletion::benchCodeCompletion():"empty":
>      1.7 msecs per iteration (total: 55, iterations: 32)
> --
> RESULT : BenchCodeCompletion::benchCodeCompletion():"stl":
>      9.7 msecs per iteration (total: 78, iterations: 8)
> --
> RESULT : BenchCodeCompletion::benchCodeCompletion():"clib":
>      18 msecs per iteration (total: 74, iterations: 4)
> 
> So from this I think it's pretty clear that we want this. The reason I write
> this lengthy mail instead of just committing

Thanks for the benchmarks. Numbers are always good.

> diff --git a/languages/clang/duchain/parsesession.cpp b/languages/clang/
> duchain/parsesession.cpp
> index f992a1c..e6c7385 100644
> --- a/languages/clang/duchain/parsesession.cpp
> +++ b/languages/clang/duchain/parsesession.cpp
> @@ -237,6 +237,7 @@ ParseSessionData::ParseSessionData(const
> QVector<UnsavedFile>& unsavedFiles, Cla
>      }
> 
>      if (m_unit) {
> +        clang_reparseTranslationUnit(m_unit, unsaved.size(),
> unsaved.data(), clang_defaultReparseOptions(m_unit));
>          setUnit(m_unit);
>          m_environment = environment;
> 
> is that it breaks quite some unit tests, even with Clang 3.7.0:
> 
> FAIL!  : TestCodeCompletion::testVariableScope() Compared values are not the
> same
>    Actual   (tester.items.size()): 3
>    Expected (4)                  : 4
>    Loc:
> [/home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/languages/
> clang/tests/test_codecompletion.cpp(988)]
> FAIL!  : TestCodeCompletion::testCompleteFunction(add-parens) Compared
> values are not the same
>    Actual   (view->document()->text()): "int foo();\nint main()
> {\nmain()\n}" Expected (expectedCode)            : "int foo();\nint main()
> {\nfoo()\n}" Loc:
> [/home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/languages/
> clang/tests/test_codecompletion.cpp(1100)]
> FAIL!  : TestCodeCompletion::testCompleteFunction(keep-parens) Compared
> values are not the same
>    Actual   (view->document()->text()): "int foo();\nint main()
> {\nfoo();\n}" Expected (expectedCode)            : "int foo();\nint main()
> {\nmain();\n}" Loc:
> [/home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/languages/
> clang/tests/test_codecompletion.cpp(1100)]
> FAIL!  : TestDUChain::testReparseUnchanged(template-default-parameters)
> 'implCtx->problems().isEmpty()' returned FALSE. ()
>    Loc:
> [/home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/languages/
> clang/tests/test_duchain.cpp(1397)]
> 
> So I think it looks like we'll have to dig into Clang to fix these issues
> and then commit the patch above. Anyone interested in helping out? Also if
> you have access to older or newer clang, please try the above and see what
> it breaks.

The testCompleteFunction is flaky for me, very likely timing related.

I had runs with zero failures, then with 1, and sometimes with the 3 failures 
you get there.
 
> Furthermore, note that we already do generate a preamble file if the user
> edits the file! So the above test issues are affecting our users *now*.
> Maybe we want to commit the patch after all?

The test case from above:

int foo();
int main()
{
    <complete here>    
    return 0;
}

... works fine for me *with* your patch in a normal KDevelop session. I get 
(foo, main) as completion items, if I just press enter I get the desired 
result: foo().

I guess its just our tests which are broken...?

Am I missing something?

Cheers,
Kevin

-- 
Kevin Funk | kfunk at kde.org | http://kfunk.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160109/dcb1a69f/attachment-0001.sig>