Author: Volodymyr Sapsai Date: 2021-11-03T17:11:14-07:00 New Revision: 0a35cc40b881a35c6a7a748f5fdefac4cafc33ce
URL: https://github.com/llvm/llvm-project/commit/0a35cc40b881a35c6a7a748f5fdefac4cafc33ce DIFF: https://github.com/llvm/llvm-project/commit/0a35cc40b881a35c6a7a748f5fdefac4cafc33ce.diff LOG: [clang][objc] Speed up populating the global method pool from modules. For each selector encountered in the source code, we need to load selectors from the imported modules and check that we are calling a selector with compatible types. At the moment, for each module we are storing methods declared in the headers belonging to this module and methods from the transitive closure of imported modules. When a module is imported by a few other modules, methods from the shared module are duplicated in each importer. As the result, we can end up with lots of identical methods that we try to add to the global method pool. Doing this duplicate work is useless and relatively expensive. Avoid processing duplicate methods by storing in each module only its own methods and not storing methods from dependencies. Collect methods from dependencies by walking the graph of module dependencies. The issue was discovered and reported by Richard Howell. He has done the hard work for this fix as he has investigated and provided a detailed explanation of the performance problem. Differential Revision: https://reviews.llvm.org/D110123 Added: clang/test/Modules/method_pool_transitive.m Modified: clang/lib/Serialization/ASTReader.cpp clang/lib/Serialization/ASTWriter.cpp clang/test/Modules/lookup.m Removed: ################################################################################ diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 0f180d821ed7f..07a0f00aa5357 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -8151,13 +8151,16 @@ namespace serialization { if (Reader.DeserializationListener) Reader.DeserializationListener->SelectorRead(Data.ID, Sel); - InstanceMethods.append(Data.Instance.begin(), Data.Instance.end()); - FactoryMethods.append(Data.Factory.begin(), Data.Factory.end()); + // Append methods in the reverse order, so that later we can process them + // in the order they appear in the source code by iterating through + // the vector in the reverse order. + InstanceMethods.append(Data.Instance.rbegin(), Data.Instance.rend()); + FactoryMethods.append(Data.Factory.rbegin(), Data.Factory.rend()); InstanceBits = Data.InstanceBits; FactoryBits = Data.FactoryBits; InstanceHasMoreThanOneDecl = Data.InstanceHasMoreThanOneDecl; FactoryHasMoreThanOneDecl = Data.FactoryHasMoreThanOneDecl; - return true; + return false; } /// Retrieve the instance methods found by this visitor. @@ -8186,9 +8189,8 @@ namespace serialization { /// Add the given set of methods to the method list. static void addMethodsToPool(Sema &S, ArrayRef<ObjCMethodDecl *> Methods, ObjCMethodList &List) { - for (unsigned I = 0, N = Methods.size(); I != N; ++I) { - S.addMethodToGlobalList(&List, Methods[I]); - } + for (auto I = Methods.rbegin(), E = Methods.rend(); I != E; ++I) + S.addMethodToGlobalList(&List, *I); } void ASTReader::ReadMethodPool(Selector Sel) { diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 7c500f30e271e..3e0e5e9c8f752 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -3045,11 +3045,11 @@ class ASTMethodPoolTrait { unsigned DataLen = 4 + 2 + 2; // 2 bytes for each of the method counts for (const ObjCMethodList *Method = &Methods.Instance; Method; Method = Method->getNext()) - if (Method->getMethod()) + if (ShouldWriteMethodListNode(Method)) DataLen += 4; for (const ObjCMethodList *Method = &Methods.Factory; Method; Method = Method->getNext()) - if (Method->getMethod()) + if (ShouldWriteMethodListNode(Method)) DataLen += 4; return emitULEBKeyDataLength(KeyLen, DataLen, Out); } @@ -3080,13 +3080,13 @@ class ASTMethodPoolTrait { unsigned NumInstanceMethods = 0; for (const ObjCMethodList *Method = &Methods.Instance; Method; Method = Method->getNext()) - if (Method->getMethod()) + if (ShouldWriteMethodListNode(Method)) ++NumInstanceMethods; unsigned NumFactoryMethods = 0; for (const ObjCMethodList *Method = &Methods.Factory; Method; Method = Method->getNext()) - if (Method->getMethod()) + if (ShouldWriteMethodListNode(Method)) ++NumFactoryMethods; unsigned InstanceBits = Methods.Instance.getBits(); @@ -3107,15 +3107,20 @@ class ASTMethodPoolTrait { LE.write<uint16_t>(FullFactoryBits); for (const ObjCMethodList *Method = &Methods.Instance; Method; Method = Method->getNext()) - if (Method->getMethod()) + if (ShouldWriteMethodListNode(Method)) LE.write<uint32_t>(Writer.getDeclID(Method->getMethod())); for (const ObjCMethodList *Method = &Methods.Factory; Method; Method = Method->getNext()) - if (Method->getMethod()) + if (ShouldWriteMethodListNode(Method)) LE.write<uint32_t>(Writer.getDeclID(Method->getMethod())); assert(Out.tell() - Start == DataLen && "Data length is wrong"); } + +private: + static bool ShouldWriteMethodListNode(const ObjCMethodList *Node) { + return (Node->getMethod() && !Node->getMethod()->isFromASTFile()); + } }; } // namespace @@ -3158,15 +3163,21 @@ void ASTWriter::WriteSelectors(Sema &SemaRef) { if (Chain && ID < FirstSelectorID) { // Selector already exists. Did it change? bool changed = false; - for (ObjCMethodList *M = &Data.Instance; - !changed && M && M->getMethod(); M = M->getNext()) { - if (!M->getMethod()->isFromASTFile()) + for (ObjCMethodList *M = &Data.Instance; M && M->getMethod(); + M = M->getNext()) { + if (!M->getMethod()->isFromASTFile()) { changed = true; + Data.Instance = *M; + break; + } } - for (ObjCMethodList *M = &Data.Factory; !changed && M && M->getMethod(); + for (ObjCMethodList *M = &Data.Factory; M && M->getMethod(); M = M->getNext()) { - if (!M->getMethod()->isFromASTFile()) + if (!M->getMethod()->isFromASTFile()) { changed = true; + Data.Factory = *M; + break; + } } if (!changed) continue; diff --git a/clang/test/Modules/lookup.m b/clang/test/Modules/lookup.m index b22e41f845942..0e09bfdd7fd95 100644 --- a/clang/test/Modules/lookup.m +++ b/clang/test/Modules/lookup.m @@ -10,8 +10,8 @@ void test(id x) { [x method]; // expected-warning@-1{{multiple methods named 'method' found}} -// expected-note@Inputs/lookup_left.h:2{{using}} -// expected-note@Inputs/lookup_right.h:3{{also found}} +// expected-note@Inputs/lookup_right.h:3{{using}} +// expected-note@Inputs/lookup_left.h:2{{also found}} } // CHECK-PRINT: - (int)method; diff --git a/clang/test/Modules/method_pool_transitive.m b/clang/test/Modules/method_pool_transitive.m new file mode 100644 index 0000000000000..40c4330b75009 --- /dev/null +++ b/clang/test/Modules/method_pool_transitive.m @@ -0,0 +1,40 @@ +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: %clang_cc1 -Wobjc-multiple-method-names -fsyntax-only -fmodules-cache-path=%t/modules.cache -fmodules -fimplicit-module-maps -F %t/Frameworks %t/test.m -verify + +// Verify we are handling methods from transitive modules, not just from immediate ones. + +//--- Frameworks/Indirect.framework/Headers/Indirect.h +@interface NSObject +@end + +@interface Indirect : NSObject +- (int)method; +@end + +//--- Frameworks/Indirect.framework/Modules/module.modulemap +framework module Indirect { + header "Indirect.h" + export * +} + +//--- Frameworks/Immediate.framework/Headers/Immediate.h +#import <Indirect/Indirect.h> +@interface Immediate : NSObject +- (void)method; +@end + +//--- Frameworks/Immediate.framework/Modules/module.modulemap +framework module Immediate { + header "Immediate.h" + export * +} + +//--- test.m +#import <Immediate/Immediate.h> + +void test(id obj) { + [obj method]; // expected-warning{{multiple methods named 'method' found}} + // expected-note@Frameworks/Indirect.framework/Headers/Indirect.h:5{{using}} + // expected-note@Frameworks/Immediate.framework/Headers/Immediate.h:3{{also found}} +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
