ping > On Mar 14, 2015, at 9:25 AM, Ben Langmuir <[email protected]> wrote: > > Hi Richard, > > 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: > > $ cat t.h > @import Cocoa; > > $ cat t.m > // empty > > $ clang -fmodules -fmodules-cache-path=mcp -x objective-c-header t.h -o t.pch > $ time clang -fmodules -fmodules-cache-path=mcp -Xclang -include-pch -Xclang > t.pch t.m -fsyntax-only > > 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. > > Do you know what’s going on here? 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. > > Ben > >> On Feb 4, 2015, at 3:37 PM, Richard Smith <[email protected]> wrote: >> >> Author: rsmith >> Date: Wed Feb 4 17:37:59 2015 >> New Revision: 228234 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=228234&view=rev >> Log: >> [modules] When using -E, we may try to merge decls despite having no Sema >> object. In such a case, use the TU's DC for merging global decls rather than >> giving up when we find there is no TU scope. >> >> Ultimately, we should probably avoid all loading of decls when preprocessing, >> but there are other reasonable use cases for loading an AST file with no Sema >> object for which this is the right thing. >> >> Added: >> cfe/trunk/test/Modules/Inputs/preprocess/ >> cfe/trunk/test/Modules/Inputs/preprocess/file.h >> cfe/trunk/test/Modules/Inputs/preprocess/fwd.h >> cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap >> Modified: >> cfe/trunk/include/clang/Frontend/CompilerInstance.h >> cfe/trunk/lib/Frontend/CompilerInstance.cpp >> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp >> cfe/trunk/test/Modules/preprocess.m >> >> Modified: cfe/trunk/include/clang/Frontend/CompilerInstance.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CompilerInstance.h?rev=228234&r1=228233&r2=228234&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/Frontend/CompilerInstance.h (original) >> +++ cfe/trunk/include/clang/Frontend/CompilerInstance.h Wed Feb 4 17:37:59 >> 2015 >> @@ -588,7 +588,7 @@ public: >> /// Create an external AST source to read a PCH file. >> /// >> /// \return - The new object on success, or null on failure. >> - static ExternalASTSource *createPCHExternalASTSource( >> + static IntrusiveRefCntPtr<ASTReader> createPCHExternalASTSource( >> StringRef Path, const std::string &Sysroot, bool DisablePCHValidation, >> bool AllowPCHWithCompilerErrors, Preprocessor &PP, ASTContext &Context, >> void *DeserializationListener, bool OwnDeserializationListener, >> >> Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=228234&r1=228233&r2=228234&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original) >> +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Wed Feb 4 17:37:59 2015 >> @@ -388,32 +388,30 @@ void CompilerInstance::createASTContext( >> void CompilerInstance::createPCHExternalASTSource( >> StringRef Path, bool DisablePCHValidation, bool >> AllowPCHWithCompilerErrors, >> void *DeserializationListener, bool OwnDeserializationListener) { >> - IntrusiveRefCntPtr<ExternalASTSource> Source; >> bool Preamble = getPreprocessorOpts().PrecompiledPreambleBytes.first != 0; >> - Source = createPCHExternalASTSource( >> + ModuleManager = createPCHExternalASTSource( >> Path, getHeaderSearchOpts().Sysroot, DisablePCHValidation, >> AllowPCHWithCompilerErrors, getPreprocessor(), getASTContext(), >> DeserializationListener, OwnDeserializationListener, Preamble, >> getFrontendOpts().UseGlobalModuleIndex); >> - ModuleManager = static_cast<ASTReader*>(Source.get()); >> - getASTContext().setExternalSource(Source); >> } >> >> -ExternalASTSource *CompilerInstance::createPCHExternalASTSource( >> +IntrusiveRefCntPtr<ASTReader> CompilerInstance::createPCHExternalASTSource( >> StringRef Path, const std::string &Sysroot, bool DisablePCHValidation, >> bool AllowPCHWithCompilerErrors, Preprocessor &PP, ASTContext &Context, >> void *DeserializationListener, bool OwnDeserializationListener, >> bool Preamble, bool UseGlobalModuleIndex) { >> HeaderSearchOptions &HSOpts = >> PP.getHeaderSearchInfo().getHeaderSearchOpts(); >> >> - std::unique_ptr<ASTReader> Reader; >> - Reader.reset(new ASTReader(PP, Context, >> - Sysroot.empty() ? "" : Sysroot.c_str(), >> - DisablePCHValidation, >> - AllowPCHWithCompilerErrors, >> - /*AllowConfigurationMismatch*/false, >> - HSOpts.ModulesValidateSystemHeaders, >> - UseGlobalModuleIndex)); >> + IntrusiveRefCntPtr<ASTReader> Reader( >> + new ASTReader(PP, Context, Sysroot.empty() ? "" : Sysroot.c_str(), >> + DisablePCHValidation, AllowPCHWithCompilerErrors, >> + /*AllowConfigurationMismatch*/ false, >> + HSOpts.ModulesValidateSystemHeaders, >> UseGlobalModuleIndex)); >> + >> + // We need the external source to be set up before we read the AST, >> because >> + // eagerly-deserialized declarations may use it. >> + Context.setExternalSource(Reader.get()); >> >> Reader->setDeserializationListener( >> static_cast<ASTDeserializationListener *>(DeserializationListener), >> @@ -427,7 +425,7 @@ ExternalASTSource *CompilerInstance::cre >> // Set the predefines buffer as suggested by the PCH reader. Typically, >> the >> // predefines buffer will be empty. >> PP.setPredefines(Reader->getSuggestedPredefines()); >> - return Reader.release(); >> + return Reader; >> >> case ASTReader::Failure: >> // Unrecoverable failure: don't even try to process the input file. >> @@ -442,6 +440,7 @@ ExternalASTSource *CompilerInstance::cre >> break; >> } >> >> + Context.setExternalSource(nullptr); >> return nullptr; >> } >> >> >> Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=228234&r1=228233&r2=228234&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original) >> +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Wed Feb 4 17:37:59 2015 >> @@ -2591,6 +2591,11 @@ DeclContext *ASTDeclReader::getPrimaryCo >> return ED->getASTContext().getLangOpts().CPlusPlus? ED->getDefinition() >> : nullptr; >> >> + // We can see the TU here only if we have no Sema object. In that case, >> + // there's no TU scope to look in, so using the DC alone is sufficient. >> + if (auto *TU = dyn_cast<TranslationUnitDecl>(DC)) >> + return TU; >> + >> return nullptr; >> } >> >> >> Added: cfe/trunk/test/Modules/Inputs/preprocess/file.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/file.h?rev=228234&view=auto >> ============================================================================== >> --- cfe/trunk/test/Modules/Inputs/preprocess/file.h (added) >> +++ cfe/trunk/test/Modules/Inputs/preprocess/file.h Wed Feb 4 17:37:59 2015 >> @@ -0,0 +1,3 @@ >> +struct __FILE; >> +#include "fwd.h" >> +typedef struct __FILE FILE; >> >> Added: cfe/trunk/test/Modules/Inputs/preprocess/fwd.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/fwd.h?rev=228234&view=auto >> ============================================================================== >> --- cfe/trunk/test/Modules/Inputs/preprocess/fwd.h (added) >> +++ cfe/trunk/test/Modules/Inputs/preprocess/fwd.h Wed Feb 4 17:37:59 2015 >> @@ -0,0 +1 @@ >> +struct __FILE; >> >> Added: cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap?rev=228234&view=auto >> ============================================================================== >> --- cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap (added) >> +++ cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap Wed Feb 4 >> 17:37:59 2015 >> @@ -0,0 +1,2 @@ >> +module fwd { header "fwd.h" export * } >> +module file { header "file.h" export * } >> >> Modified: cfe/trunk/test/Modules/preprocess.m >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/preprocess.m?rev=228234&r1=228233&r2=228234&view=diff >> ============================================================================== >> --- cfe/trunk/test/Modules/preprocess.m (original) >> +++ cfe/trunk/test/Modules/preprocess.m Wed Feb 4 17:37:59 2015 >> @@ -1,9 +1,14 @@ >> // RUN: rm -rf %t >> -// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -include >> %S/Inputs/preprocess-prefix.h -E %s | FileCheck -strict-whitespace %s >> -// 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 >> -// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs >> -include-pch %t.pch -E %s | FileCheck -strict-whitespace %s >> +// 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 >> +// 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 >> +// 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 >> +// >> +// 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 >> +// 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 >> +// 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 >> #import "diamond_right.h" >> #import "diamond_right.h" // to check that imports get their own line >> +#include "file.h" >> void test() { >> top_left_before(); >> left_and_right(); >> @@ -15,6 +20,7 @@ void test() { >> >> // CHECK: @import diamond_right; /* clang -E: implicit import for >> "{{.*}}diamond_right.h" */{{$}} >> // CHECK: @import diamond_right; /* clang -E: implicit import for >> "{{.*}}diamond_right.h" */{{$}} >> +// CHECK: @import file; /* clang -E: implicit import for "{{.*}}file.h" >> */{{$}} >> // CHECK-NEXT: void test() {{{$}} >> // CHECK-NEXT: top_left_before();{{$}} >> // CHECK-NEXT: left_and_right();{{$}} >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
