<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Mar 14, 2015 at 9:25 AM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Richard,<br>
<br>
This commit looks like it caused a regression in PCH-loading performance when the PCH imports modules. It doesn’t affect performance of loading modules on their own, or of a PCH that doesn’t use modules. If you have access to a Darwin system, it’s easy to reproduce with:<br>
<br>
$ cat t.h<br>
@import Cocoa;<br>
<br>
$ cat t.m<br>
// empty<br>
<br>
$ clang -fmodules -fmodules-cache-path=mcp -x objective-c-header t.h -o t.pch<br>
$ time clang -fmodules -fmodules-cache-path=mcp -Xclang -include-pch -Xclang t.pch t.m -fsyntax-only<br>
<br>
I’m seeing ~8x difference between r228233 and r228234. In real world code (where the .m file isn’t empty) we’ve seen up to a ~90% difference.<br>
<br>
Do you know what’s going on here?</blockquote><div><br></div><div>Hmm. I suspect this change causes the lookup table for the translation unit to be built, which in turn pulls in all the lexical contents of the translation unit (because outside of C++ we don't serialize a DeclContext lookup table for the TU, nor even build one usually).</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">When a PCH loads I think we do it without Sema, but (at least previously) queued up the interesting Decls we saw for when Sema was added later. So I’m surprised this had such a big effect.</blockquote><div><br></div><div>If we were loading PCHs without Sema, then I think before this change we would not have merged declarations of the same entity from two independent modules imported into the same PCH.</div><div><br></div><div>Perhaps we should use a separate side-table for merging in this case outside C++, rather than falling back to DeclContext lookup?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="HOEnZb"><font color="#888888">
Ben<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> On Feb 4, 2015, at 3:37 PM, Richard Smith <<a href="mailto:richard-llvm@metafoo.co.uk">richard-llvm@metafoo.co.uk</a>> wrote:<br>
><br>
> Author: rsmith<br>
> Date: Wed Feb 4 17:37:59 2015<br>
> New Revision: 228234<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=228234&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=228234&view=rev</a><br>
> Log:<br>
> [modules] When using -E, we may try to merge decls despite having no Sema<br>
> object. In such a case, use the TU's DC for merging global decls rather than<br>
> giving up when we find there is no TU scope.<br>
><br>
> Ultimately, we should probably avoid all loading of decls when preprocessing,<br>
> but there are other reasonable use cases for loading an AST file with no Sema<br>
> object for which this is the right thing.<br>
><br>
> Added:<br>
> cfe/trunk/test/Modules/Inputs/preprocess/<br>
> cfe/trunk/test/Modules/Inputs/preprocess/file.h<br>
> cfe/trunk/test/Modules/Inputs/preprocess/fwd.h<br>
> cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap<br>
> Modified:<br>
> cfe/trunk/include/clang/Frontend/CompilerInstance.h<br>
> cfe/trunk/lib/Frontend/CompilerInstance.cpp<br>
> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp<br>
> cfe/trunk/test/Modules/preprocess.m<br>
><br>
> Modified: cfe/trunk/include/clang/Frontend/CompilerInstance.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CompilerInstance.h?rev=228234&r1=228233&r2=228234&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CompilerInstance.h?rev=228234&r1=228233&r2=228234&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/Frontend/CompilerInstance.h (original)<br>
> +++ cfe/trunk/include/clang/Frontend/CompilerInstance.h Wed Feb 4 17:37:59 2015<br>
> @@ -588,7 +588,7 @@ public:<br>
> /// Create an external AST source to read a PCH file.<br>
> ///<br>
> /// \return - The new object on success, or null on failure.<br>
> - static ExternalASTSource *createPCHExternalASTSource(<br>
> + static IntrusiveRefCntPtr<ASTReader> createPCHExternalASTSource(<br>
> StringRef Path, const std::string &Sysroot, bool DisablePCHValidation,<br>
> bool AllowPCHWithCompilerErrors, Preprocessor &PP, ASTContext &Context,<br>
> void *DeserializationListener, bool OwnDeserializationListener,<br>
><br>
> Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=228234&r1=228233&r2=228234&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=228234&r1=228233&r2=228234&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original)<br>
> +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Wed Feb 4 17:37:59 2015<br>
> @@ -388,32 +388,30 @@ void CompilerInstance::createASTContext(<br>
> void CompilerInstance::createPCHExternalASTSource(<br>
> StringRef Path, bool DisablePCHValidation, bool AllowPCHWithCompilerErrors,<br>
> void *DeserializationListener, bool OwnDeserializationListener) {<br>
> - IntrusiveRefCntPtr<ExternalASTSource> Source;<br>
> bool Preamble = getPreprocessorOpts().PrecompiledPreambleBytes.first != 0;<br>
> - Source = createPCHExternalASTSource(<br>
> + ModuleManager = createPCHExternalASTSource(<br>
> Path, getHeaderSearchOpts().Sysroot, DisablePCHValidation,<br>
> AllowPCHWithCompilerErrors, getPreprocessor(), getASTContext(),<br>
> DeserializationListener, OwnDeserializationListener, Preamble,<br>
> getFrontendOpts().UseGlobalModuleIndex);<br>
> - ModuleManager = static_cast<ASTReader*>(Source.get());<br>
> - getASTContext().setExternalSource(Source);<br>
> }<br>
><br>
> -ExternalASTSource *CompilerInstance::createPCHExternalASTSource(<br>
> +IntrusiveRefCntPtr<ASTReader> CompilerInstance::createPCHExternalASTSource(<br>
> StringRef Path, const std::string &Sysroot, bool DisablePCHValidation,<br>
> bool AllowPCHWithCompilerErrors, Preprocessor &PP, ASTContext &Context,<br>
> void *DeserializationListener, bool OwnDeserializationListener,<br>
> bool Preamble, bool UseGlobalModuleIndex) {<br>
> HeaderSearchOptions &HSOpts = PP.getHeaderSearchInfo().getHeaderSearchOpts();<br>
><br>
> - std::unique_ptr<ASTReader> Reader;<br>
> - Reader.reset(new ASTReader(PP, Context,<br>
> - Sysroot.empty() ? "" : Sysroot.c_str(),<br>
> - DisablePCHValidation,<br>
> - AllowPCHWithCompilerErrors,<br>
> - /*AllowConfigurationMismatch*/false,<br>
> - HSOpts.ModulesValidateSystemHeaders,<br>
> - UseGlobalModuleIndex));<br>
> + IntrusiveRefCntPtr<ASTReader> Reader(<br>
> + new ASTReader(PP, Context, Sysroot.empty() ? "" : Sysroot.c_str(),<br>
> + DisablePCHValidation, AllowPCHWithCompilerErrors,<br>
> + /*AllowConfigurationMismatch*/ false,<br>
> + HSOpts.ModulesValidateSystemHeaders, UseGlobalModuleIndex));<br>
> +<br>
> + // We need the external source to be set up before we read the AST, because<br>
> + // eagerly-deserialized declarations may use it.<br>
> + Context.setExternalSource(Reader.get());<br>
><br>
> Reader->setDeserializationListener(<br>
> static_cast<ASTDeserializationListener *>(DeserializationListener),<br>
> @@ -427,7 +425,7 @@ ExternalASTSource *CompilerInstance::cre<br>
> // Set the predefines buffer as suggested by the PCH reader. Typically, the<br>
> // predefines buffer will be empty.<br>
> PP.setPredefines(Reader->getSuggestedPredefines());<br>
> - return Reader.release();<br>
> + return Reader;<br>
><br>
> case ASTReader::Failure:<br>
> // Unrecoverable failure: don't even try to process the input file.<br>
> @@ -442,6 +440,7 @@ ExternalASTSource *CompilerInstance::cre<br>
> break;<br>
> }<br>
><br>
> + Context.setExternalSource(nullptr);<br>
> return nullptr;<br>
> }<br>
><br>
><br>
> Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=228234&r1=228233&r2=228234&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=228234&r1=228233&r2=228234&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)<br>
> +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Wed Feb 4 17:37:59 2015<br>
> @@ -2591,6 +2591,11 @@ DeclContext *ASTDeclReader::getPrimaryCo<br>
> return ED->getASTContext().getLangOpts().CPlusPlus? ED->getDefinition()<br>
> : nullptr;<br>
><br>
> + // We can see the TU here only if we have no Sema object. In that case,<br>
> + // there's no TU scope to look in, so using the DC alone is sufficient.<br>
> + if (auto *TU = dyn_cast<TranslationUnitDecl>(DC))<br>
> + return TU;<br>
> +<br>
> return nullptr;<br>
> }<br>
><br>
><br>
> Added: cfe/trunk/test/Modules/Inputs/preprocess/file.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/file.h?rev=228234&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/file.h?rev=228234&view=auto</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/Modules/Inputs/preprocess/file.h (added)<br>
> +++ cfe/trunk/test/Modules/Inputs/preprocess/file.h Wed Feb 4 17:37:59 2015<br>
> @@ -0,0 +1,3 @@<br>
> +struct __FILE;<br>
> +#include "fwd.h"<br>
> +typedef struct __FILE FILE;<br>
><br>
> Added: cfe/trunk/test/Modules/Inputs/preprocess/fwd.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/fwd.h?rev=228234&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/fwd.h?rev=228234&view=auto</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/Modules/Inputs/preprocess/fwd.h (added)<br>
> +++ cfe/trunk/test/Modules/Inputs/preprocess/fwd.h Wed Feb 4 17:37:59 2015<br>
> @@ -0,0 +1 @@<br>
> +struct __FILE;<br>
><br>
> Added: cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap?rev=228234&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap?rev=228234&view=auto</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap (added)<br>
> +++ cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap Wed Feb 4 17:37:59 2015<br>
> @@ -0,0 +1,2 @@<br>
> +module fwd { header "fwd.h" export * }<br>
> +module file { header "file.h" export * }<br>
><br>
> Modified: cfe/trunk/test/Modules/preprocess.m<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/preprocess.m?rev=228234&r1=228233&r2=228234&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/preprocess.m?rev=228234&r1=228233&r2=228234&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/Modules/preprocess.m (original)<br>
> +++ cfe/trunk/test/Modules/preprocess.m Wed Feb 4 17:37:59 2015<br>
> @@ -1,9 +1,14 @@<br>
> // RUN: rm -rf %t<br>
> -// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -include %S/Inputs/preprocess-prefix.h -E %s | FileCheck -strict-whitespace %s<br>
> -// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -x objective-c-header -emit-pch %S/Inputs/preprocess-prefix.h -o %t.pch<br>
> -// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -include-pch %t.pch -E %s | FileCheck -strict-whitespace %s<br>
> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -I %S/Inputs/preprocess -include %S/Inputs/preprocess-prefix.h -E %s | FileCheck -strict-whitespace %s<br>
> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -I %S/Inputs/preprocess -x objective-c-header -emit-pch %S/Inputs/preprocess-prefix.h -o %t.pch<br>
> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -I %S/Inputs/preprocess -include-pch %t.pch -E %s | FileCheck -strict-whitespace %s<br>
> +//<br>
> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -I %S/Inputs/preprocess -x objective-c++ -include %S/Inputs/preprocess-prefix.h -E %s | FileCheck -strict-whitespace %s<br>
> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -I %S/Inputs/preprocess -x objective-c++-header -emit-pch %S/Inputs/preprocess-prefix.h -o %t.pch<br>
> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -I %S/Inputs/preprocess -x objective-c++ -include-pch %t.pch -E %s | FileCheck -strict-whitespace %s<br>
> #import "diamond_right.h"<br>
> #import "diamond_right.h" // to check that imports get their own line<br>
> +#include "file.h"<br>
> void test() {<br>
> top_left_before();<br>
> left_and_right();<br>
> @@ -15,6 +20,7 @@ void test() {<br>
><br>
> // CHECK: @import diamond_right; /* clang -E: implicit import for "{{.*}}diamond_right.h" */{{$}}<br>
> // CHECK: @import diamond_right; /* clang -E: implicit import for "{{.*}}diamond_right.h" */{{$}}<br>
> +// CHECK: @import file; /* clang -E: implicit import for "{{.*}}file.h" */{{$}}<br>
> // CHECK-NEXT: void test() {{{$}}<br>
> // CHECK-NEXT: top_left_before();{{$}}<br>
> // CHECK-NEXT: left_and_right();{{$}}<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
</div></div></blockquote></div><br></div></div>