[PATCH] D117460: [clang-tidy][NFC] Reduce map lookups in IncludeSorter

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

This change is dangerous, it depends that elements/iterators won't be 
re-allocated/invalidated during insertion process.
Part with removing double lookup in IncludeSorter::addInclude looks fine, but 
part that involve IncludeBucket is problematic.
I will try to push part that involve IncludeLocations optimizations.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117460/new/

https://reviews.llvm.org/D117460

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117460: [clang-tidy][NFC] Reduce map lookups in IncludeSorter

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.
Herald added a project: All.

Are we waiting on some sort of benchmark before accepting/rejecting this change?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117460/new/

https://reviews.llvm.org/D117460

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117460: [clang-tidy][NFC] Reduce map lookups in IncludeSorter

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D117460#3248613 , @alexfh wrote:

> I wonder what motivated the patch. Is this a performance optimization? If so, 
> do you have any measurements?

I was doing some work on IncludeInserter and just thought, this seems 
unnecessary. It'd be hard to microbenchmark this, but I'll see what I can 
rustle up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117460/new/

https://reviews.llvm.org/D117460

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117460: [clang-tidy][NFC] Reduce map lookups in IncludeSorter

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

I wonder what motivated the patch. Is this a performance optimization? If so, 
do you have any measurements?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117460/new/

https://reviews.llvm.org/D117460

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117460: [clang-tidy][NFC] Reduce map lookups in IncludeSorter

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: LegalizeAdulthood, alexfh, aaron.ballman.
Herald added subscribers: carlosgalvezp, xazax.hun.
njames93 requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Because StringMapEntrys have stable addresses, The IncludeBuckets can be 
changed to store pointers to entrys instead of the key used for accessing 
IncludeLocations.
This saves having to lookup the IncludeLocations map as well as creating 
unnecessary strings.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D117460

Files:
  clang-tools-extra/clang-tidy/utils/IncludeSorter.cpp
  clang-tools-extra/clang-tidy/utils/IncludeSorter.h


Index: clang-tools-extra/clang-tidy/utils/IncludeSorter.h
===
--- clang-tools-extra/clang-tidy/utils/IncludeSorter.h
+++ clang-tools-extra/clang-tidy/utils/IncludeSorter.h
@@ -10,7 +10,6 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_INCLUDESORTER_H
 
 #include "../ClangTidyCheck.h"
-#include 
 
 namespace clang {
 namespace tidy {
@@ -61,7 +60,8 @@
   /// Mapping from file name to #include locations.
   llvm::StringMap IncludeLocations;
   /// Includes sorted into buckets.
-  SmallVector IncludeBucket[IK_InvalidInclude];
+  SmallVector *, 1>
+  IncludeBucket[IK_InvalidInclude];
 };
 
 } // namespace utils
Index: clang-tools-extra/clang-tidy/utils/IncludeSorter.cpp
===
--- clang-tools-extra/clang-tidy/utils/IncludeSorter.cpp
+++ clang-tools-extra/clang-tidy/utils/IncludeSorter.cpp
@@ -134,20 +134,21 @@
SourceLocation EndLocation) {
   int Offset = findNextLine(SourceMgr->getCharacterData(EndLocation));
 
+  auto &MapEntry = *IncludeLocations.try_emplace(FileName).first;
   // Record the relevant location information for this inclusion directive.
-  IncludeLocations[FileName].push_back(
+  MapEntry.getValue().push_back(
   SourceRange(HashLocation, EndLocation.getLocWithOffset(Offset)));
-  SourceLocations.push_back(IncludeLocations[FileName].back());
+  SourceLocations.push_back(MapEntry.getValue().back());
 
   // Stop if this inclusion is a duplicate.
-  if (IncludeLocations[FileName].size() > 1)
+  if (MapEntry.getValue().size() > 1)
 return;
 
   // Add the included file's name to the appropriate bucket.
   IncludeKinds Kind =
   determineIncludeKind(CanonicalFile, FileName, IsAngled, Style);
   if (Kind != IK_InvalidInclude)
-IncludeBucket[Kind].push_back(FileName.str());
+IncludeBucket[Kind].push_back(&MapEntry);
 }
 
 Optional IncludeSorter::createIncludeInsertion(StringRef FileName,
@@ -174,9 +175,11 @@
   determineIncludeKind(CanonicalFile, FileName, IsAngled, Style);
 
   if (!IncludeBucket[IncludeKind].empty()) {
-for (const std::string &IncludeEntry : IncludeBucket[IncludeKind]) {
+for (auto *IncludeMapEntry : IncludeBucket[IncludeKind]) {
+  StringRef IncludeEntry = IncludeMapEntry->getKey();
   if (compareHeaders(FileName, IncludeEntry, Style) < 0) {
-const auto &Location = IncludeLocations[IncludeEntry][0];
+
+const auto &Location = IncludeMapEntry->getValue().front();
 return FixItHint::CreateInsertion(Location.getBegin(), IncludeStmt);
   }
   if (FileName == IncludeEntry) {
@@ -185,8 +188,8 @@
 }
 // FileName comes after all include entries in bucket, insert it after
 // last.
-const std::string &LastInclude = IncludeBucket[IncludeKind].back();
-SourceRange LastIncludeLocation = IncludeLocations[LastInclude].back();
+SourceRange LastIncludeLocation =
+IncludeBucket[IncludeKind].back()->getValue().back();
 return FixItHint::CreateInsertion(LastIncludeLocation.getEnd(),
   IncludeStmt);
   }
@@ -209,15 +212,15 @@
 
   if (NonEmptyKind < IncludeKind) {
 // Create a block after.
-const std::string &LastInclude = IncludeBucket[NonEmptyKind].back();
-SourceRange LastIncludeLocation = IncludeLocations[LastInclude].back();
+SourceRange LastIncludeLocation =
+IncludeBucket[NonEmptyKind].back()->getValue().back();
 IncludeStmt = '\n' + IncludeStmt;
 return FixItHint::CreateInsertion(LastIncludeLocation.getEnd(),
   IncludeStmt);
   }
   // Create a block before.
-  const std::string &FirstInclude = IncludeBucket[NonEmptyKind][0];
-  SourceRange FirstIncludeLocation = IncludeLocations[FirstInclude].back();
+  SourceRange FirstIncludeLocation =
+  IncludeBucket[NonEmptyKind][0]->getValue().back();
   IncludeStmt.append("\n");
   return FixItHint::CreateInsertion(FirstIncludeLocation.getBegin(),
 IncludeStmt);


Index: clang-tools-extra/clang-tidy/utils/IncludeSorter.h
===
--- clang-tools-extra/clang-tidy/utils/