Skip to content

Commit 73ea64f

Browse files
sam-mccalltru
authored andcommitted
[clangd] Avoid scanning up to end of file on each comment!
Assigning char* (pointing at comment start) to StringRef was causing us to scan the rest of the source file looking for the null terminator. This seems to be eating about 8% of our *total* CPU! While fixing this, factor out the common bits from the two places we're parsing IWYU pragmas. Differential Revision: https://reviews.llvm.org/D135314 (cherry picked from commit 5d2d527)
1 parent 61fa709 commit 73ea64f

File tree

4 files changed

+43
-19
lines changed

4 files changed

+43
-19
lines changed

clang-tools-extra/clangd/Headers.cpp

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,17 @@
2222
namespace clang {
2323
namespace clangd {
2424

25-
const char IWYUPragmaKeep[] = "// IWYU pragma: keep";
26-
const char IWYUPragmaExport[] = "// IWYU pragma: export";
27-
const char IWYUPragmaBeginExports[] = "// IWYU pragma: begin_exports";
25+
llvm::Optional<StringRef> parseIWYUPragma(const char *Text) {
26+
// This gets called for every comment seen in the preamble, so it's quite hot.
27+
constexpr llvm::StringLiteral IWYUPragma = "// IWYU pragma: ";
28+
if (strncmp(Text, IWYUPragma.data(), IWYUPragma.size()))
29+
return llvm::None;
30+
Text += IWYUPragma.size();
31+
const char *End = Text;
32+
while (*End != 0 && *End != '\n')
33+
++End;
34+
return StringRef(Text, End - Text);
35+
}
2836

2937
class IncludeStructure::RecordHeaders : public PPCallbacks,
3038
public CommentHandler {
@@ -129,10 +137,10 @@ class IncludeStructure::RecordHeaders : public PPCallbacks,
129137
}
130138

131139
bool HandleComment(Preprocessor &PP, SourceRange Range) override {
132-
bool Err = false;
133-
llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), &Err);
134-
if (Err)
140+
auto Pragma = parseIWYUPragma(SM.getCharacterData(Range.getBegin()));
141+
if (!Pragma)
135142
return false;
143+
136144
if (inMainFile()) {
137145
// Given:
138146
//
@@ -150,8 +158,7 @@ class IncludeStructure::RecordHeaders : public PPCallbacks,
150158
// will know that the next inclusion is behind the IWYU pragma.
151159
// FIXME: Support "IWYU pragma: begin_exports" and "IWYU pragma:
152160
// end_exports".
153-
if (!Text.startswith(IWYUPragmaExport) &&
154-
!Text.startswith(IWYUPragmaKeep))
161+
if (!Pragma->startswith("export") && !Pragma->startswith("keep"))
155162
return false;
156163
unsigned Offset = SM.getFileOffset(Range.getBegin());
157164
LastPragmaKeepInMainFileLine =
@@ -161,8 +168,7 @@ class IncludeStructure::RecordHeaders : public PPCallbacks,
161168
// does not support them properly yet, so they will be not marked as
162169
// unused.
163170
// FIXME: Once IncludeCleaner supports export pragmas, remove this.
164-
if (!Text.startswith(IWYUPragmaExport) &&
165-
!Text.startswith(IWYUPragmaBeginExports))
171+
if (!Pragma->startswith("export") && !Pragma->startswith("begin_exports"))
166172
return false;
167173
Out->HasIWYUExport.insert(
168174
*Out->getID(SM.getFileEntryForID(SM.getFileID(Range.getBegin()))));

clang-tools-extra/clangd/Headers.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,12 @@ namespace clangd {
3535
/// Returns true if \p Include is literal include like "path" or <path>.
3636
bool isLiteralInclude(llvm::StringRef Include);
3737

38+
/// If Text begins an Include-What-You-Use directive, returns it.
39+
/// Given "// IWYU pragma: keep", returns "keep".
40+
/// Input is a null-terminated char* as provided by SM.getCharacterData().
41+
/// (This should not be StringRef as we do *not* want to scan for its length).
42+
llvm::Optional<StringRef> parseIWYUPragma(const char *Text);
43+
3844
/// Represents a header file to be #include'd.
3945
struct HeaderFile {
4046
std::string File;

clang-tools-extra/clangd/index/CanonicalIncludes.cpp

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,6 @@
1717
namespace clang {
1818
namespace clangd {
1919
namespace {
20-
const char IWYUPragma[] = "// IWYU pragma: private, include ";
21-
2220
const std::pair<llvm::StringRef, llvm::StringRef> IncludeMappings[] = {
2321
{"include/__stddef_max_align_t.h", "<cstddef>"},
2422
{"include/__wmmintrin_aes.h", "<wmmintrin.h>"},
@@ -712,17 +710,17 @@ collectIWYUHeaderMaps(CanonicalIncludes *Includes) {
712710
PragmaCommentHandler(CanonicalIncludes *Includes) : Includes(Includes) {}
713711

714712
bool HandleComment(Preprocessor &PP, SourceRange Range) override {
715-
llvm::StringRef Text =
716-
Lexer::getSourceText(CharSourceRange::getCharRange(Range),
717-
PP.getSourceManager(), PP.getLangOpts());
718-
if (!Text.consume_front(IWYUPragma))
713+
auto Pragma = parseIWYUPragma(
714+
PP.getSourceManager().getCharacterData(Range.getBegin()));
715+
if (!Pragma || !Pragma->consume_front("private, include "))
719716
return false;
720717
auto &SM = PP.getSourceManager();
721718
// We always insert using the spelling from the pragma.
722719
if (auto *FE = SM.getFileEntryForID(SM.getFileID(Range.getBegin())))
723-
Includes->addMapping(
724-
FE->getLastRef(),
725-
isLiteralInclude(Text) ? Text.str() : ("\"" + Text + "\"").str());
720+
Includes->addMapping(FE->getLastRef(),
721+
isLiteralInclude(*Pragma)
722+
? Pragma->str()
723+
: ("\"" + *Pragma + "\"").str());
726724
return false;
727725
}
728726

clang-tools-extra/clangd/unittests/HeadersTests.cpp

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "Headers.h"
1010

1111
#include "Compiler.h"
12+
#include "Matchers.h"
1213
#include "TestFS.h"
1314
#include "TestTU.h"
1415
#include "clang/Basic/TokenKinds.h"
@@ -30,6 +31,7 @@ namespace {
3031
using ::testing::AllOf;
3132
using ::testing::Contains;
3233
using ::testing::ElementsAre;
34+
using ::testing::Eq;
3335
using ::testing::IsEmpty;
3436
using ::testing::Not;
3537
using ::testing::UnorderedElementsAre;
@@ -445,6 +447,18 @@ TEST_F(HeadersTest, HasIWYUPragmas) {
445447
EXPECT_FALSE(Includes.hasIWYUExport(getID("none.h", Includes)));
446448
}
447449

450+
TEST(Headers, ParseIWYUPragma) {
451+
EXPECT_THAT(parseIWYUPragma("// IWYU pragma: keep"), HasValue(Eq("keep")));
452+
EXPECT_THAT(parseIWYUPragma("// IWYU pragma: keep\netc"),
453+
HasValue(Eq("keep")));
454+
EXPECT_EQ(parseIWYUPragma("/* IWYU pragma: keep"), llvm::None)
455+
<< "Only // comments supported!";
456+
EXPECT_EQ(parseIWYUPragma("// IWYU pragma: keep"), llvm::None)
457+
<< "Sensitive to whitespace";
458+
EXPECT_EQ(parseIWYUPragma("// IWYU pragma:keep"), llvm::None)
459+
<< "Sensitive to whitespace";
460+
}
461+
448462
} // namespace
449463
} // namespace clangd
450464
} // namespace clang

0 commit comments

Comments
 (0)