<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Mar 23, 2015, at 1:05 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" class="">richard@metafoo.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_extra"><div class="gmail_quote">On Sat, Mar 14, 2015 at 9:25 AM, Ben Langmuir<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:blangmuir@apple.com" target="_blank" class="">blangmuir@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;">Hi Richard,<br class=""><br class="">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 class=""><br class="">$ cat t.h<br class="">@import Cocoa;<br class=""><br class="">$ cat t.m<br class="">// empty<br class=""><br class="">$ clang -fmodules -fmodules-cache-path=mcp -x objective-c-header t.h -o t.pch<br class="">$ time clang -fmodules -fmodules-cache-path=mcp -Xclang -include-pch -Xclang t.pch t.m -fsyntax-only<br class=""><br class="">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 class=""><br class="">Do you know what’s going on here?</blockquote><div class=""><br class=""></div><div class="">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 class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: 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 class=""><br class=""></div><div class="">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></div></div></div></blockquote><div><br class=""></div><div>Wouldn’t that happen in InitializeSema() when we called pushExternalDeclIntoScope() on the contents of PreloadedDeclIDs?</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><div class="">Perhaps we should use a separate side-table for merging in this case outside C++, rather than falling back to DeclContext lookup?</div></div></div></div></div></blockquote><div><br class=""></div><div>I don’t know this code path well enough to say. What would go into the table (or perhaps more importantly what wouldn’t)?</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span class="HOEnZb"><font color="#888888" class="">Ben<br class=""></font></span><div class="HOEnZb"><div class="h5"><br class="">> On Feb 4, 2015, at 3:37 PM, Richard Smith <<a href="mailto:richard-llvm@metafoo.co.uk" class="">richard-llvm@metafoo.co.uk</a>> wrote:<br class="">><br class="">> Author: rsmith<br class="">> Date: Wed Feb 4 17:37:59 2015<br class="">> New Revision: 228234<br class="">><br class="">> URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project?rev=228234&view=rev" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=228234&view=rev</a><br class="">> Log:<br class="">> [modules] When using -E, we may try to merge decls despite having no Sema<br class="">> object. In such a case, use the TU's DC for merging global decls rather than<br class="">> giving up when we find there is no TU scope.<br class="">><br class="">> Ultimately, we should probably avoid all loading of decls when preprocessing,<br class="">> but there are other reasonable use cases for loading an AST file with no Sema<br class="">> object for which this is the right thing.<br class="">><br class="">> Added:<br class="">> cfe/trunk/test/Modules/Inputs/preprocess/<br class="">> cfe/trunk/test/Modules/Inputs/preprocess/file.h<br class="">> cfe/trunk/test/Modules/Inputs/preprocess/fwd.h<br class="">> cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap<br class="">> Modified:<br class="">> cfe/trunk/include/clang/Frontend/CompilerInstance.h<br class="">> cfe/trunk/lib/Frontend/CompilerInstance.cpp<br class="">> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp<br class="">> cfe/trunk/test/Modules/preprocess.m<br class="">><br class="">> Modified: cfe/trunk/include/clang/Frontend/CompilerInstance.h<br class="">> URL:<span class="Apple-converted-space"> </span><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" class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CompilerInstance.h?rev=228234&r1=228233&r2=228234&view=diff</a><br class="">> ==============================================================================<br class="">> --- cfe/trunk/include/clang/Frontend/CompilerInstance.h (original)<br class="">> +++ cfe/trunk/include/clang/Frontend/CompilerInstance.h Wed Feb 4 17:37:59 2015<br class="">> @@ -588,7 +588,7 @@ public:<br class="">> /// Create an external AST source to read a PCH file.<br class="">> ///<br class="">> /// \return - The new object on success, or null on failure.<br class="">> - static ExternalASTSource *createPCHExternalASTSource(<br class="">> + static IntrusiveRefCntPtr<ASTReader> createPCHExternalASTSource(<br class="">> StringRef Path, const std::string &Sysroot, bool DisablePCHValidation,<br class="">> bool AllowPCHWithCompilerErrors, Preprocessor &PP, ASTContext &Context,<br class="">> void *DeserializationListener, bool OwnDeserializationListener,<br class="">><br class="">> Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp<br class="">> URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=228234&r1=228233&r2=228234&view=diff" target="_blank" class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=228234&r1=228233&r2=228234&view=diff</a><br class="">> ==============================================================================<br class="">> --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original)<br class="">> +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Wed Feb 4 17:37:59 2015<br class="">> @@ -388,32 +388,30 @@ void CompilerInstance::createASTContext(<br class="">> void CompilerInstance::createPCHExternalASTSource(<br class="">> StringRef Path, bool DisablePCHValidation, bool AllowPCHWithCompilerErrors,<br class="">> void *DeserializationListener, bool OwnDeserializationListener) {<br class="">> - IntrusiveRefCntPtr<ExternalASTSource> Source;<br class="">> bool Preamble = getPreprocessorOpts().PrecompiledPreambleBytes.first != 0;<br class="">> - Source = createPCHExternalASTSource(<br class="">> + ModuleManager = createPCHExternalASTSource(<br class="">> Path, getHeaderSearchOpts().Sysroot, DisablePCHValidation,<br class="">> AllowPCHWithCompilerErrors, getPreprocessor(), getASTContext(),<br class="">> DeserializationListener, OwnDeserializationListener, Preamble,<br class="">> getFrontendOpts().UseGlobalModuleIndex);<br class="">> - ModuleManager = static_cast<ASTReader*>(Source.get());<br class="">> - getASTContext().setExternalSource(Source);<br class="">> }<br class="">><br class="">> -ExternalASTSource *CompilerInstance::createPCHExternalASTSource(<br class="">> +IntrusiveRefCntPtr<ASTReader> CompilerInstance::createPCHExternalASTSource(<br class="">> StringRef Path, const std::string &Sysroot, bool DisablePCHValidation,<br class="">> bool AllowPCHWithCompilerErrors, Preprocessor &PP, ASTContext &Context,<br class="">> void *DeserializationListener, bool OwnDeserializationListener,<br class="">> bool Preamble, bool UseGlobalModuleIndex) {<br class="">> HeaderSearchOptions &HSOpts = PP.getHeaderSearchInfo().getHeaderSearchOpts();<br class="">><br class="">> - std::unique_ptr<ASTReader> Reader;<br class="">> - Reader.reset(new ASTReader(PP, Context,<br class="">> - Sysroot.empty() ? "" : Sysroot.c_str(),<br class="">> - DisablePCHValidation,<br class="">> - AllowPCHWithCompilerErrors,<br class="">> - /*AllowConfigurationMismatch*/false,<br class="">> - HSOpts.ModulesValidateSystemHeaders,<br class="">> - UseGlobalModuleIndex));<br class="">> + IntrusiveRefCntPtr<ASTReader> Reader(<br class="">> + new ASTReader(PP, Context, Sysroot.empty() ? "" : Sysroot.c_str(),<br class="">> + DisablePCHValidation, AllowPCHWithCompilerErrors,<br class="">> + /*AllowConfigurationMismatch*/ false,<br class="">> + HSOpts.ModulesValidateSystemHeaders, UseGlobalModuleIndex));<br class="">> +<br class="">> + // We need the external source to be set up before we read the AST, because<br class="">> + // eagerly-deserialized declarations may use it.<br class="">> + Context.setExternalSource(Reader.get());<br class="">><br class="">> Reader->setDeserializationListener(<br class="">> static_cast<ASTDeserializationListener *>(DeserializationListener),<br class="">> @@ -427,7 +425,7 @@ ExternalASTSource *CompilerInstance::cre<br class="">> // Set the predefines buffer as suggested by the PCH reader. Typically, the<br class="">> // predefines buffer will be empty.<br class="">> PP.setPredefines(Reader->getSuggestedPredefines());<br class="">> - return Reader.release();<br class="">> + return Reader;<br class="">><br class="">> case ASTReader::Failure:<br class="">> // Unrecoverable failure: don't even try to process the input file.<br class="">> @@ -442,6 +440,7 @@ ExternalASTSource *CompilerInstance::cre<br class="">> break;<br class="">> }<br class="">><br class="">> + Context.setExternalSource(nullptr);<br class="">> return nullptr;<br class="">> }<br class="">><br class="">><br class="">> Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp<br class="">> URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=228234&r1=228233&r2=228234&view=diff" target="_blank" class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=228234&r1=228233&r2=228234&view=diff</a><br class="">> ==============================================================================<br class="">> --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)<br class="">> +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Wed Feb 4 17:37:59 2015<br class="">> @@ -2591,6 +2591,11 @@ DeclContext *ASTDeclReader::getPrimaryCo<br class="">> return ED->getASTContext().getLangOpts().CPlusPlus? ED->getDefinition()<br class="">> : nullptr;<br class="">><br class="">> + // We can see the TU here only if we have no Sema object. In that case,<br class="">> + // there's no TU scope to look in, so using the DC alone is sufficient.<br class="">> + if (auto *TU = dyn_cast<TranslationUnitDecl>(DC))<br class="">> + return TU;<br class="">> +<br class="">> return nullptr;<br class="">> }<br class="">><br class="">><br class="">> Added: cfe/trunk/test/Modules/Inputs/preprocess/file.h<br class="">> URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/file.h?rev=228234&view=auto" target="_blank" class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/file.h?rev=228234&view=auto</a><br class="">> ==============================================================================<br class="">> --- cfe/trunk/test/Modules/Inputs/preprocess/file.h (added)<br class="">> +++ cfe/trunk/test/Modules/Inputs/preprocess/file.h Wed Feb 4 17:37:59 2015<br class="">> @@ -0,0 +1,3 @@<br class="">> +struct __FILE;<br class="">> +#include "fwd.h"<br class="">> +typedef struct __FILE FILE;<br class="">><br class="">> Added: cfe/trunk/test/Modules/Inputs/preprocess/fwd.h<br class="">> URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/fwd.h?rev=228234&view=auto" target="_blank" class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/fwd.h?rev=228234&view=auto</a><br class="">> ==============================================================================<br class="">> --- cfe/trunk/test/Modules/Inputs/preprocess/fwd.h (added)<br class="">> +++ cfe/trunk/test/Modules/Inputs/preprocess/fwd.h Wed Feb 4 17:37:59 2015<br class="">> @@ -0,0 +1 @@<br class="">> +struct __FILE;<br class="">><br class="">> Added: cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap<br class="">> URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap?rev=228234&view=auto" target="_blank" class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap?rev=228234&view=auto</a><br class="">> ==============================================================================<br class="">> --- cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap (added)<br class="">> +++ cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap Wed Feb 4 17:37:59 2015<br class="">> @@ -0,0 +1,2 @@<br class="">> +module fwd { header "fwd.h" export * }<br class="">> +module file { header "file.h" export * }<br class="">><br class="">> Modified: cfe/trunk/test/Modules/preprocess.m<br class="">> URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/preprocess.m?rev=228234&r1=228233&r2=228234&view=diff" target="_blank" class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/preprocess.m?rev=228234&r1=228233&r2=228234&view=diff</a><br class="">> ==============================================================================<br class="">> --- cfe/trunk/test/Modules/preprocess.m (original)<br class="">> +++ cfe/trunk/test/Modules/preprocess.m Wed Feb 4 17:37:59 2015<br class="">> @@ -1,9 +1,14 @@<br class="">> // RUN: rm -rf %t<br class="">> -// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -include %S/Inputs/preprocess-prefix.h -E %s | FileCheck -strict-whitespace %s<br class="">> -// 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 class="">> -// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -include-pch %t.pch -E %s | FileCheck -strict-whitespace %s<br class="">> +// 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 class="">> +// 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 class="">> +// 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 class="">> +//<br class="">> +// 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 class="">> +// 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 class="">> +// 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 class="">> #import "diamond_right.h"<br class="">> #import "diamond_right.h" // to check that imports get their own line<br class="">> +#include "file.h"<br class="">> void test() {<br class="">> top_left_before();<br class="">> left_and_right();<br class="">> @@ -15,6 +20,7 @@ void test() {<br class="">><br class="">> // CHECK: @import diamond_right; /* clang -E: implicit import for "{{.*}}diamond_right.h" */{{$}}<br class="">> // CHECK: @import diamond_right; /* clang -E: implicit import for "{{.*}}diamond_right.h" */{{$}}<br class="">> +// CHECK: @import file; /* clang -E: implicit import for "{{.*}}file.h" */{{$}}<br class="">> // CHECK-NEXT: void test() {{{$}}<br class="">> // CHECK-NEXT: top_left_before();{{$}}<br class="">> // CHECK-NEXT: left_and_right();{{$}}<br class="">><br class="">><br class="">> _______________________________________________<br class="">> cfe-commits mailing list<br class="">><span class="Apple-converted-space"> </span><a href="mailto:cfe-commits@cs.uiuc.edu" class="">cfe-commits@cs.uiuc.edu</a><br class="">><span class="Apple-converted-space"> </span><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a></div></div></blockquote></div></div></div></div></blockquote></div><br class=""></body></html>