D18551: clang: Create preamble only on second parse

[Milian Wolff]

mwolff added a comment.


  In D18551#544467 <https://phabricator.kde.org/D18551#544467>, @aaronpuchert wrote:
  
  > I still think this is the right change, but if there is no consensus I will implement @mwolff's suggestion. That would certainly be an improvement, but it doesn't really solve my problem. I'm currently working on Clang and have around 6 files open, which is really not a lot. I haven't changed any of them. The result:
  >
  >   aaron:~/src/llvm-project> ls -l /tmp/preamble-*
  >   -rw-r--r-- 1 aaron users 73139496 Okt  9 22:27 /tmp/preamble-0e19f4.pch
  >   -rw-r--r-- 1 aaron users 59327864 Okt  9 22:23 /tmp/preamble-2a515c.pch
  >   -rw-r--r-- 1 aaron users 61885536 Okt  9 22:23 /tmp/preamble-2baca3.pch
  >   -rw-r--r-- 1 aaron users 76913424 Okt  9 22:23 /tmp/preamble-9853df.pch
  >   -rw-r--r-- 1 aaron users 64254872 Okt  9 22:23 /tmp/preamble-cbb035.pch
  >   -rw-r--r-- 1 aaron users 64829792 Okt  9 22:24 /tmp/preamble-dd9bf1.pch
  >   aaron:~/src/llvm-project> du -s /tmp/ 2>/dev/null
  >   391204  /tmp/
  >
  >
  > Now I actually have `/tmp` as tmpfs, and so these 400 MB don't come for free. This is perfectly fine when I'm actually working on these files, but so far I have only opened KDevelop.
  
  
  I see. The problem really is that the preamble files shouldn't be written to /tmp then - it would be perfectly fine to move them to a temp dir on the disk I think - it would still be faster than having to redo the work.
  
  > In D18551#412611 <https://phabricator.kde.org/D18551#412611>, @mwolff wrote:
  > 
  >> In D18551#410223 <https://phabricator.kde.org/D18551#410223>, @aaronpuchert wrote:
  >>
  >> > In D18551#409551 <https://phabricator.kde.org/D18551#409551>, @mwolff wrote:
  >> >
  >> > > When we didn't create the preamble, we will have to reparse the file completely. And this can easily take 1-2s per file, which is *really* annoying.
  >> >
  >> >
  >> > It only happens on the very first edit though, and will only be noticeable if code completion is required immediately after starting to edit the file. All following parses and code completions can use the precompiled preamble then. I don't think that's too bad. It's also not unusual, many applications have something like a "warmup phase" where not everything is loaded yet.
  >>
  >>
  >> Yes, but hiding or minimizing the warmup phase is good. And it would be possible by having the flag active for opened files like I request.
  > 
  > 
  > Creating a preamble is actually more expensive than not creating it. So we make the initial parsing slower. This is only faster for files are actually edited.
  
  Yes, that's why I definitely agree that we should not do it for files that aren't opened in an editor.
  
  > Creating it immediately only improves the reaction time for the first edit. For that small benefit, we're paying a hefty price.
  
  Yes, I agree that in your case the price is high and the benefit small. In my case it usually isn't that big a deal, but YMMV :) And I'm all for improving this state somehow.
  
  >>>> Also note how we just reparsed the file solely for code completion purposes!
  >>> 
  >>> You mean the preamble? Since code completion is usually invoked after some initial changes, we have to reparse anyway. What you said would only be true if we invoke code completion without changing anything first, which seems rather weird.
  >> 
  >> I mean: We trigger a reparse when we open a document, to get hold of the clang TU to do code completion. If this *does not* create a preamble, then the following call to query code completion items - *after* editing - will create the preamble. The preamble only includes the headers of the file, not the contents of the `.cpp`.
  > 
  > Well, then why don't we just skip the initial reparse, and only reparse after the first edit?
  
  Yes, I agree that we should try this out. Note that this will require changes in `clang/codecompletion/model.cpp` at least, potentially also in other places. I.e. right now this code relies on the fact that we trigger a reparse after opening a document. This then allows us to write code like this there:
  
    auto top = DUChainUtils::standardContextForUrl(m_url);
    if (!top) {
        qCWarning(KDEV_CLANG) << "No context found for" << m_url;
        return;
    }
    
    ParseSessionData::Ptr sessionData(ClangIntegration::DUChainUtils::findParseSessionData(top->url(), m_index->translationUnitForUrl(top->url())));
    
    if (!sessionData) {
        // TODO: trigger reparse and re-request code completion
        qCWarning(KDEV_CLANG) << "No parse session / AST attached to context for url" << m_url;
        return;
    }
  
  If we only attach the AST after the first edit, it won't be enough. We will also have to trigger a reparse and force attaching the AST/parse session data when code completion is explicitly requested via `ctrl + space` without editing.
  
  >> You usually edit the `.cpp` file and then you want to get code completion results immediately.
  > 
  > We're talking about a one-time delay, all further edits will have a preamble.
  
  Yes, but the patch proposed here in its current form just disables the preamble always, no? So I think we agree that this isn't good enough?
  
  >> Having a preamble is crucial for good performance here.
  > 
  > I'm not suggesting to not create a preamble at all, but to create it only when we **know** it is needed.
  
  OK, then I think we are on the same page - just this patch needs more love :)

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D18551

To: aaronpuchert, #kdevelop, mwolff, brauch, rjvbb
Cc: rjvbb, kdevelop-devel, hmitonneau, christiant, glebaccon, domson, antismap, iodelay, alexeymin, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20200131/80890d69/attachment-0001.html>