r228234 - [modules] When using -E, we may try to merge decls despite having no Sema
Richard Smith
richard at metafoo.co.uk
Mon Mar 23 13:05:30 PDT 2015
On Sat, Mar 14, 2015 at 9:25 AM, Ben Langmuir <blangmuir at apple.com> 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?
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).
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.
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.
Perhaps we should use a separate side-table for merging in this case
outside C++, rather than falling back to DeclContext lookup?
Ben
>
> > On Feb 4, 2015, at 3:37 PM, Richard Smith <richard-llvm at metafoo.co.uk>
> 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
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150323/9c159b0c/attachment.html>
More information about the cfe-commits
mailing list