From 1083a10a66b34626a1055a2c107f2cc6e24dc0ab Mon Sep 17 00:00:00 2001 From: Jacob Dufault Date: Tue, 9 May 2017 21:52:15 -0700 Subject: [PATCH] Fix some threading bugs with code completion --- src/code_completion.cc | 193 +++++++++++++++-------------------------- src/code_completion.h | 12 ++- src/command_line.cc | 3 +- src/working_files.cc | 19 +++- src/working_files.h | 10 ++- 5 files changed, 107 insertions(+), 130 deletions(-) diff --git a/src/code_completion.cc b/src/code_completion.cc index 8601bf81..14b3752f 100644 --- a/src/code_completion.cc +++ b/src/code_completion.cc @@ -9,28 +9,37 @@ namespace { unsigned Flags() { - /* - return - //CXTranslationUnit_Incomplete | - CXTranslationUnit_PrecompiledPreamble | - CXTranslationUnit_CacheCompletionResults | - //CXTranslationUnit_ForSerialization | - CXTranslationUnit_IncludeBriefCommentsInCodeCompletion | - //CXTranslationUnit_CreatePreambleOnFirstParse | - CXTranslationUnit_KeepGoing; - */ - return + CXTranslationUnit_Incomplete | + CXTranslationUnit_KeepGoing | CXTranslationUnit_CacheCompletionResults | CXTranslationUnit_PrecompiledPreamble | - CXTranslationUnit_IncludeBriefCommentsInCodeCompletion | + CXTranslationUnit_IncludeBriefCommentsInCodeCompletion #if !defined(_WIN32) // For whatever reason, CreatePreambleOnFirstParse causes clang to become // very crashy on windows. // TODO: do more investigation, submit fixes to clang. - CXTranslationUnit_CreatePreambleOnFirstParse | + | CXTranslationUnit_CreatePreambleOnFirstParse #endif - CXTranslationUnit_DetailedPreprocessingRecord; + ; +} + +int GetCompletionPriority(const CXCompletionString& str, CXCursorKind result_kind, const std::string& label) { + int priority = clang_getCompletionPriority(str); + if (result_kind == CXCursor_Destructor) { + priority *= 100; + //std::cerr << "Bumping[destructor] " << ls_completion_item.label << std::endl; + } + if (result_kind == CXCursor_ConversionFunction || + (result_kind == CXCursor_CXXMethod && StartsWith(label, "operator"))) { + //std::cerr << "Bumping[conversion] " << ls_completion_item.label << std::endl; + priority *= 100; + } + if (clang_getCompletionAvailability(str) != CXAvailability_Available) { + //std::cerr << "Bumping[notavailable] " << ls_completion_item.label << std::endl; + priority *= 100; + } + return priority; } lsCompletionItemKind GetCompletionKind(CXCursorKind cursor_kind) { @@ -96,7 +105,7 @@ lsCompletionItemKind GetCompletionKind(CXCursorKind cursor_kind) { return lsCompletionItemKind::Text; default: - std::cerr << "Unhandled completion kind " << cursor_kind << std::endl; + std::cerr << "[complete] Unhandled completion kind " << cursor_kind << std::endl; return lsCompletionItemKind::Text; } } @@ -201,29 +210,29 @@ std::string BuildDetailString(CXCompletionString completion_string) { void CompletionMain(CompletionManager* completion_manager) { while (true) { + // Fetching the completion request blocks until we have a request. std::unique_ptr request = completion_manager->completion_request.Take(); NonElidedVector ls_result; CompletionSession* session = completion_manager->GetOrOpenSession(request->location.textDocument.uri.GetPath()); + std::lock_guard lock(session->usage_lock); + + session->EnsureCompletionState(); unsigned line = request->location.position.line + 1; unsigned column = request->location.position.character + 1; std::cerr << std::endl; - std::cerr << "Completing at " << line << ":" << column << std::endl; - - // TODO/FIXME - // TODO/FIXME - // TODO/FIXME NOT THREAD SAFE - // TODO/FIXME - // TODO/FIXME - std::vector unsaved = completion_manager->working_files->AsUnsavedFiles(); - + std::cerr << "[complete] Completing at " << line << ":" << column << std::endl; Timer timer; + std::vector unsaved = completion_manager->working_files->AsUnsavedFiles(); + timer.ResetAndPrint("[complete] Fetching unsaved files"); + + timer.Reset(); CXCodeCompleteResults* cx_results = clang_codeCompleteAt( @@ -232,14 +241,13 @@ void CompletionMain(CompletionManager* completion_manager) { unsaved.data(), unsaved.size(), CXCodeComplete_IncludeMacros | CXCodeComplete_IncludeBriefComments); if (!cx_results) { - std::cerr << "Code completion failed" << std::endl; + std::cerr << "[complete] Code completion failed" << std::endl; request->on_complete(ls_result); continue; } - timer.ResetAndPrint("clangCodeCompleteAt"); - std::cerr << "Got " << cx_results->NumResults << " results" << std::endl; - // TODO: for comments we could hack the unsaved buffer and transform // into /// + timer.ResetAndPrint("[complete] clangCodeCompleteAt"); + std::cerr << "[complete] Got " << cx_results->NumResults << " results" << std::endl; ls_result.reserve(cx_results->NumResults); @@ -247,130 +255,58 @@ void CompletionMain(CompletionManager* completion_manager) { for (unsigned i = 0; i < cx_results->NumResults; ++i) { CXCompletionResult& result = cx_results->Results[i]; - //unsigned int is_incomplete; - //CXCursorKind kind = clang_codeCompleteGetContainerKind(cx_results, &is_incomplete); - //std::cerr << "clang_codeCompleteGetContainerKind kind=" << kind << " is_incomplete=" << is_incomplete << std::endl; - - // CXCursor_InvalidCode fo - - //clang_codeCompleteGetContexts - //CXCompletionContext - // clang_codeCompleteGetContexts - // - //CXCursorKind kind; - //CXString str = clang_getCompletionParent(result.CompletionString, &kind); - //std::cerr << "clang_getCompletionParent kind=" << kind << ", str=" << clang::ToString(str) << std::endl; - // if global don't append now, append to end later - - // also clang_codeCompleteGetContainerKind - // TODO: fill in more data lsCompletionItem ls_completion_item; - // kind/label/detail/docs + // kind/label/detail/docs/sortText ls_completion_item.kind = GetCompletionKind(result.CursorKind); ls_completion_item.label = BuildLabelString(result.CompletionString); ls_completion_item.detail = BuildDetailString(result.CompletionString); ls_completion_item.documentation = clang::ToString(clang_getCompletionBriefComment(result.CompletionString)); - - // Priority - int priority = clang_getCompletionPriority(result.CompletionString); - if (result.CursorKind == CXCursor_Destructor) { - priority *= 100; - //std::cerr << "Bumping[destructor] " << ls_completion_item.label << std::endl; - } - if (result.CursorKind == CXCursor_ConversionFunction || - (result.CursorKind == CXCursor_CXXMethod && StartsWith(ls_completion_item.label, "operator"))) { - //std::cerr << "Bumping[conversion] " << ls_completion_item.label << std::endl; - priority *= 100; - } - if (clang_getCompletionAvailability(result.CompletionString) != CXAvailability_Available) { - //std::cerr << "Bumping[notavailable] " << ls_completion_item.label << std::endl; - priority *= 100; - } - //std::cerr << "Adding kind=" << result.CursorKind << ", priority=" << ls_completion_item.priority_ << ", label=" << ls_completion_item.label << std::endl; - - // TODO: we can probably remove priority_ and our sort. - ls_completion_item.sortText = uint64_t(priority);// std::to_string(ls_completion_item.priority_); + ls_completion_item.sortText = uint64_t(GetCompletionPriority(result.CompletionString, result.CursorKind, ls_completion_item.label)); ls_result.push_back(ls_completion_item); } - timer.ResetAndPrint("Building " + std::to_string(ls_result.size()) + " completion results"); + timer.ResetAndPrint("[complete] Building " + std::to_string(ls_result.size()) + " completion results"); clang_disposeCodeCompleteResults(cx_results); - timer.ResetAndPrint("clang_disposeCodeCompleteResults "); + timer.ResetAndPrint("[complete] clang_disposeCodeCompleteResults "); request->on_complete(ls_result); continue; - - // we should probably main two translation units, one for - // serving current requests, and one that is reparsing (follow qtcreator) - - // todo: we need to run code completion on a separate thread from querydb - // so thread layout looks like: - // - stdin # Reads data from stdin - // - stdout # Pushes data to stdout - // - querydb # Resolves index database queries. - // - complete_responder # Handles low-latency code complete requests. - // - complete_parser # Parses most recent document for future code complete requests. - // - indexer (many) # Runs index jobs (for querydb updates) - - // use clang_codeCompleteAt - //CXUnsavedFile - // we need to setup CXUnsavedFile - // The key to doing that is via - // - textDocument/didOpen - // - textDocument/didChange - // - textDocument/didClose - - // probably don't need - // - textDocument/willSave } } } // namespace -CompletionSession::CompletionSession(const Project::Entry& file, WorkingFiles* working_files) : file(file) { - std::vector unsaved = working_files->AsUnsavedFiles(); +CompletionSession::CompletionSession(const Project::Entry& file, WorkingFiles* working_files) + : file(file), working_files(working_files) {} + +CompletionSession::~CompletionSession() {} + +void CompletionSession::EnsureCompletionState() { + if (active && active_index) { + // TODO: Investigate if this helps performance. It causes crashes on windows. + //std::vector unsaved = working_files->AsUnsavedFiles(); + //active->ReparseTranslationUnit(unsaved); + return; + } std::vector args = file.args; args.push_back("-fparse-all-comments"); - std::cerr << "Creating completion session with arguments " << StringJoin(args) << std::endl; + std::vector unsaved = working_files->AsUnsavedFiles(); - // TODO: I think we crash when there are syntax errors. + std::cerr << "[complete] Creating completion session with arguments " << StringJoin(args) << std::endl; active_index = MakeUnique(0 /*excludeDeclarationsFromPCH*/, 0 /*displayDiagnostics*/); active = MakeUnique(*active_index, file.filename, args, unsaved, Flags()); - std::cerr << "Done creating active; did_fail=" << active->did_fail << std::endl; - //if (active->did_fail) { - // std::cerr << "Failed to create translation unit; trying again..." << std::endl; - // active = MakeUnique(*active_index, file.filename, args, unsaved, Flags()); - //} - - // Despite requesting clang create a precompiled header on the first parse in - // Flags() via CXTranslationUnit_CreatePreambleOnFirstParse, it doesn't seem - // to do so. Immediately reparsing will create one which reduces - // clang_codeCompleteAt timing from 200ms to 20ms on simple files. - // TODO: figure out why this is crashing so much - if (!active->did_fail) { - std::cerr << "Start reparse" << std::endl; - active->ReparseTranslationUnit(unsaved); - - std::cerr << "Done reparse" << std::endl; - } -} - -CompletionSession::~CompletionSession() {} - -void CompletionSession::Refresh(std::vector& unsaved) { - // TODO: Do this off the code completion thread so we don't block completions. - active->ReparseTranslationUnit(unsaved); + std::cerr << "[complete] Done creating active; did_fail=" << active->did_fail << std::endl; } CompletionManager::CompletionManager(IndexerConfig* config, Project* project, WorkingFiles* working_files) : config(config), project(project), working_files(working_files) { new std::thread([&]() { - SetCurrentThreadName("completion"); + SetCurrentThreadName("complete"); CompletionMain(this); }); } @@ -390,16 +326,27 @@ CompletionSession* CompletionManager::GetOrOpenSession(const std::string& filena } // Create new session. Note that this will block. - std::cerr << "Creating new code completion session for " << filename << std::endl; + std::cerr << "[complete] Creating new code completion session for " << filename << std::endl; optional entry = project->FindCompilationEntryForFile(filename); if (!entry) { - std::cerr << "Unable to find compilation entry" << std::endl; + std::cerr << "[complete] Unable to find compilation entry" << std::endl; entry = Project::Entry(); entry->filename = filename; } else { - std::cerr << "Found compilation entry" << std::endl; + std::cerr << "[complete] Found compilation entry" << std::endl; } sessions.push_back(MakeUnique(*entry, working_files)); return sessions[sessions.size() - 1].get(); } + +void CompletionManager::DropAllSessionsExcept(const std::string& filename) { + for (auto& session : sessions) { + if (session->file.filename == filename) + continue; + + std::lock_guard lock(session->usage_lock); + session->active.reset(); + session->active_index.reset(); + } +} \ No newline at end of file diff --git a/src/code_completion.h b/src/code_completion.h index 56ff02c0..a9f8b1ed 100644 --- a/src/code_completion.h +++ b/src/code_completion.h @@ -8,9 +8,14 @@ #include #include +#include struct CompletionSession { Project::Entry file; + WorkingFiles* working_files; + + // Acquired when the session is being used. + std::mutex usage_lock; // The active translation unit. std::unique_ptr active; @@ -18,6 +23,7 @@ struct CompletionSession { // Updated translation unit. If |is_updated_ready| is true, then |updated| // contains more recent state than |active| and the two should be swapped. + // // TODO: implement this. Needs changes in Refresh and CodeComplete. //bool is_updated_ready = false; //std::unique_ptr updated; @@ -26,8 +32,8 @@ struct CompletionSession { CompletionSession(const Project::Entry& file, WorkingFiles* working_files); ~CompletionSession(); - // Refresh file index. - void Refresh(std::vector& unsaved); + // Validate that we have |active| and |active_index|. + void EnsureCompletionState(); }; struct CompletionManager { @@ -41,6 +47,7 @@ struct CompletionManager { lsTextDocumentPositionParams location; OnComplete on_complete; }; + AtomicObject completion_request; CompletionManager(IndexerConfig* config, Project* project, WorkingFiles* working_files); @@ -50,4 +57,5 @@ struct CompletionManager { void CodeComplete(const lsTextDocumentPositionParams& completion_location, const OnComplete& on_complete); CompletionSession* GetOrOpenSession(const std::string& filename); + void DropAllSessionsExcept(const std::string& filename); }; \ No newline at end of file diff --git a/src/command_line.cc b/src/command_line.cc index a7ec22fc..ea657840 100644 --- a/src/command_line.cc +++ b/src/command_line.cc @@ -1534,6 +1534,7 @@ bool QueryDbMainLoop( working_file->pending_new_index_content = working_file->buffer_content; queue_do_index->Enqueue(Index_DoIndex(Index_DoIndex::Type::Parse, project->FindCompilationEntryForFile(path))); } + completion_manager->DropAllSessionsExcept(path); break; } @@ -1579,7 +1580,7 @@ bool QueryDbMainLoop( Timer timer; ipc->SendOutMessageToClient(IpcId::TextDocumentCompletion, response); - timer.ResetAndPrint("Writing completion results"); + timer.ResetAndPrint("[complete] Writing completion results"); delete message; }, message.release(), std::placeholders::_1); diff --git a/src/working_files.cc b/src/working_files.cc index 5c01b4b3..3d30f15f 100644 --- a/src/working_files.cc +++ b/src/working_files.cc @@ -151,6 +151,11 @@ CXUnsavedFile WorkingFile::AsUnsavedFile() const { } WorkingFile* WorkingFiles::GetFileByFilename(const std::string& filename) { + std::lock_guard lock(files_mutex); + return GetFileByFilenameNoLock(filename); +} + +WorkingFile* WorkingFiles::GetFileByFilenameNoLock(const std::string& filename) { for (auto& file : files) { if (file->filename == filename) return file.get(); @@ -159,11 +164,13 @@ WorkingFile* WorkingFiles::GetFileByFilename(const std::string& filename) { } WorkingFile* WorkingFiles::OnOpen(const Ipc_TextDocumentDidOpen::Params& open) { + std::lock_guard lock(files_mutex); + std::string filename = open.textDocument.uri.GetPath(); std::string content = open.textDocument.text; // The file may already be open. - if (WorkingFile* file = GetFileByFilename(filename)) { + if (WorkingFile* file = GetFileByFilenameNoLock(filename)) { file->version = open.textDocument.version; file->buffer_content = content; file->OnBufferContentUpdated(); @@ -175,8 +182,10 @@ WorkingFile* WorkingFiles::OnOpen(const Ipc_TextDocumentDidOpen::Params& open) { } void WorkingFiles::OnChange(const Ipc_TextDocumentDidChange::Params& change) { + std::lock_guard lock(files_mutex); + std::string filename = change.textDocument.uri.GetPath(); - WorkingFile* file = GetFileByFilename(filename); + WorkingFile* file = GetFileByFilenameNoLock(filename); if (!file) { std::cerr << "Could not change " << filename << " because it was not open" << std::endl; return; @@ -205,6 +214,8 @@ void WorkingFiles::OnChange(const Ipc_TextDocumentDidChange::Params& change) { } void WorkingFiles::OnClose(const Ipc_TextDocumentDidClose::Params& close) { + std::lock_guard lock(files_mutex); + std::string filename = close.textDocument.uri.GetPath(); for (int i = 0; i < files.size(); ++i) { @@ -217,7 +228,9 @@ void WorkingFiles::OnClose(const Ipc_TextDocumentDidClose::Params& close) { std::cerr << "Could not close " << filename << " because it was not open" << std::endl; } -std::vector WorkingFiles::AsUnsavedFiles() const { +std::vector WorkingFiles::AsUnsavedFiles() { + std::lock_guard lock(files_mutex); + std::vector result; result.reserve(files.size()); for (auto& file : files) diff --git a/src/working_files.h b/src/working_files.h index 497bc771..7547d67b 100644 --- a/src/working_files.h +++ b/src/working_files.h @@ -6,6 +6,7 @@ #include #include +#include #include using std::experimental::optional; @@ -52,15 +53,22 @@ struct WorkingFile { }; struct WorkingFiles { + // + // :: IMPORTANT :: All methods in this class are guarded by a single lock. + // + // Find the file with the given filename. WorkingFile* GetFileByFilename(const std::string& filename); + WorkingFile* GetFileByFilenameNoLock(const std::string& filename); + WorkingFile* OnOpen(const Ipc_TextDocumentDidOpen::Params& open); void OnChange(const Ipc_TextDocumentDidChange::Params& change); void OnClose(const Ipc_TextDocumentDidClose::Params& close); - std::vector AsUnsavedFiles() const; + std::vector AsUnsavedFiles(); // Use unique_ptrs so we can handout WorkingFile ptrs and not have them // invalidated if we resize files. std::vector> files; + std::mutex files_mutex; // Protects |files|. }; \ No newline at end of file