r179730 - [Modules] Use global index to improve typo correction performance

[Argyrios Kyrtzidis]

On Apr 17, 2013, at 4:07 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Wed, Apr 17, 2013 at 3:10 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>> Author: akirtzidis
>> Date: Wed Apr 17 17:10:55 2013
>> New Revision: 179730
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=179730&view=rev
>> Log:
>> [Modules] Use global index to improve typo correction performance
>> 
>> Typo correction for an unqualified name needs to walk through all of the identifier tables of all modules.
>> When we have a global index, just walk its identifier table only.
>> 
>> rdar://13425732
>> 
>> Modified:
>>    cfe/trunk/include/clang/Basic/IdentifierTable.h
>>    cfe/trunk/include/clang/Serialization/ASTReader.h
>>    cfe/trunk/include/clang/Serialization/GlobalModuleIndex.h
>>    cfe/trunk/lib/Basic/IdentifierTable.cpp
>>    cfe/trunk/lib/Serialization/ASTReader.cpp
>>    cfe/trunk/lib/Serialization/GlobalModuleIndex.cpp
>> 
>> Modified: cfe/trunk/include/clang/Basic/IdentifierTable.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/IdentifierTable.h?rev=179730&r1=179729&r2=179730&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/IdentifierTable.h (original)
>> +++ cfe/trunk/include/clang/Basic/IdentifierTable.h Wed Apr 17 17:10:55 2013
>> @@ -394,7 +394,7 @@ public:
>>   ///
>>   /// \returns A new iterator into the set of known identifiers. The
>>   /// caller is responsible for deleting this iterator.
>> -  virtual IdentifierIterator *getIdentifiers() const;
>> +  virtual IdentifierIterator *getIdentifiers();
>> };
>> 
>> /// \brief An abstract class used to resolve numerical identifier
>> 
>> Modified: cfe/trunk/include/clang/Serialization/ASTReader.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTReader.h?rev=179730&r1=179729&r2=179730&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Serialization/ASTReader.h (original)
>> +++ cfe/trunk/include/clang/Serialization/ASTReader.h Wed Apr 17 17:10:55 2013
>> @@ -1558,7 +1558,7 @@ public:
>> 
>>   /// \brief Retrieve an iterator into the set of all identifiers
>>   /// in all loaded AST files.
>> -  virtual IdentifierIterator *getIdentifiers() const;
>> +  virtual IdentifierIterator *getIdentifiers();
>> 
>>   /// \brief Load the contents of the global method pool for a given
>>   /// selector.
>> 
>> Modified: cfe/trunk/include/clang/Serialization/GlobalModuleIndex.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/GlobalModuleIndex.h?rev=179730&r1=179729&r2=179730&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Serialization/GlobalModuleIndex.h (original)
>> +++ cfe/trunk/include/clang/Serialization/GlobalModuleIndex.h Wed Apr 17 17:10:55 2013
>> @@ -146,6 +146,11 @@ public:
>>   static std::pair<GlobalModuleIndex *, ErrorCode>
>>   readIndex(StringRef Path);
>> 
>> +  /// \brief Returns an iterator for identifiers stored in the index table.
>> +  ///
>> +  /// The caller accepts ownership of the returned object.
>> +  IdentifierIterator *createIdentifierIterator() const;
>> +
>>   /// \brief Retrieve the set of modules that have up-to-date indexes.
>>   ///
>>   /// \param ModuleFiles Will be populated with the set of module files that
>> 
>> Modified: cfe/trunk/lib/Basic/IdentifierTable.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/IdentifierTable.cpp?rev=179730&r1=179729&r2=179730&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Basic/IdentifierTable.cpp (original)
>> +++ cfe/trunk/lib/Basic/IdentifierTable.cpp Wed Apr 17 17:10:55 2013
>> @@ -65,7 +65,7 @@ namespace {
>>   };
>> }
>> 
>> -IdentifierIterator *IdentifierInfoLookup::getIdentifiers() const {
>> +IdentifierIterator *IdentifierInfoLookup::getIdentifiers() {
>>   return new EmptyLookupIterator();
>> }
>> 
>> 
>> Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=179730&r1=179729&r2=179730&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
>> +++ cfe/trunk/lib/Serialization/ASTReader.cpp Wed Apr 17 17:10:55 2013
>> @@ -6109,7 +6109,10 @@ StringRef ASTIdentifierIterator::Next()
>>   return Result;
>> }
>> 
>> -IdentifierIterator *ASTReader::getIdentifiers() const {
>> +IdentifierIterator *ASTReader::getIdentifiers() {
>> +  if (!loadGlobalIndex())
>> +    return GlobalIndex->createIdentifierIterator();
> 
> This sort of feels like it still ought to be conceptually const,
> perhaps? (ie: loading the global index (I haven't actually looked at
> the implementation details of that function) is an implementation
> detail that doesn't change the "state" of the ASTReader, does it?
> Well, I suppose it's a 'reader' so its constness is non-obvious to
> begin with). Just a thought.

This is implementing the method from the abstract class IdentifierInfoLookup (ASTReader is an implementation of it).
Not a strong opinion but I'm leaning towards that IdentifierInfoLookup should not dictate to its implementations that the "getIdentifiers()" method implementation ought to be const. It did not for the "get()" method.

> 
>> +
>>   return new ASTIdentifierIterator(*this);
>> }
>> 
>> 
>> Modified: cfe/trunk/lib/Serialization/GlobalModuleIndex.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/GlobalModuleIndex.cpp?rev=179730&r1=179729&r2=179730&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Serialization/GlobalModuleIndex.cpp (original)
>> +++ cfe/trunk/lib/Serialization/GlobalModuleIndex.cpp Wed Apr 17 17:10:55 2013
>> @@ -818,3 +818,34 @@ GlobalModuleIndex::writeIndex(FileManage
>>   // We're done.
>>   return EC_None;
>> }
>> +
>> +namespace {
>> +  class GlobalIndexIdentifierIterator : public IdentifierIterator {
>> +    /// \brief The current position within the identifier lookup table.
>> +    IdentifierIndexTable::key_iterator Current;
>> +
>> +    /// \brief The end position within the identifier lookup table.
>> +    IdentifierIndexTable::key_iterator End;
>> +
>> +  public:
>> +    explicit GlobalIndexIdentifierIterator(IdentifierIndexTable &Idx) {
>> +      Current = Idx.key_begin();
>> +      End = Idx.key_end();
>> +    }
>> +
>> +    virtual StringRef Next() {
>> +      if (Current == End)
>> +        return StringRef();
>> +
>> +      StringRef Result = *Current;
>> +      ++Current;
>> +      return Result;
>> +    }
>> +  };
>> +}
>> +
>> +IdentifierIterator *GlobalModuleIndex::createIdentifierIterator() const {
>> +  IdentifierIndexTable &Table =
>> +    *static_cast<IdentifierIndexTable *>(IdentifierIndex);
>> +  return new GlobalIndexIdentifierIterator(Table);
>> +}
>> 
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits