[NFC][Lexer] Make Lexer::LangOpts const reference

[yurai007]

Summary

This change can be seen as code cleanup but motivation is more performance related.
While browsing perf reports captured during Linux build we can notice unusual portion of instructions executed in std::vector<std::string> copy constructor like:

0.59%     0.58%  clang-14    clang-14      [.] std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >,
                                                                std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::vector

or even:

1.42%     0.26%  clang    clang-14             [.] clang::LangOptions::LangOptions
       |
        --1.16%--clang::LangOptions::LangOptions
                  |
                   --0.74%--std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >,
                            std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::vector

After more digging we can see that relevant LangOptions std::vector members (*Files, ModuleFeatures and NoBuiltinFuncs)
are constructed when Lexer::LangOpts field is initialized on list:

Lexer::Lexer(..., const LangOptions &langOpts, ...)
            : ..., LangOpts(langOpts),

Since LangOptions copy constructor is called by Lexer(..., const LangOptions &LangOpts,...) and local Lexer objects are created thousands times
(in Lexer::getRawToken, Preprocessor::EnterSourceFile and more) during single module processing in frontend it makes std::vector copy constructors surprisingly hot.

Unfortunately even though in current Lexer implementation mentioned std::vector members are unused and most of time empty,
no compiler is smart enough to optimize their std::vector copy constructors out (take a look at test assembly): https://godbolt.org/z/hdoxPfMYY even with LTO enabled.
However there is simple way to fix this. Since Lexer doesn't access *Files, ModuleFeatures, NoBuiltinFuncs and any other LangOptions fields (but only LangOptionsBase)
we can simply get rid of redundant copy constructor assembly by changing LangOpts type to more appropriate const LangOptions reference: https://godbolt.org/z/fP7de9176

Additionally we need to store LineComment outside LangOpts because it's written in SkipLineComment function.
Also FormatTokenLexer need to be adjusted a bit to avoid lifetime issues related to passing local LangOpts reference to Lexer.

After this change I can see more than 1% speedup in some of my microbenchmarks when using Clang release binary built with LTO.
For Linux build gains are not so significant but still nice at the level of -0.4%/-0.5% instructions drop.

[jansvoboda11]

The performance implications are pretty interesting! Have you tried avoiding the copy altogether by just storing the reference?

[yurai007]

In D120334#3337961, @jansvoboda11 wrote:

The performance implications are pretty interesting! Have you tried avoiding the copy altogether by just storing the reference?

Yes, actually that was my first attempt but I failed miserably. LangOpts::LineComment is mutated in Lexer so LangOpts cannot be simply const&. Making it non-const reference is technically doable with some extra adjustment for Lexer constructors.
However I'm worried a bit how it would impact external LangOpts users after mutating LineComment member by Lexer. As long as LangOpts acts as simple cache it's easy to reason about but when we start to share it with Lexer callers I'm not longer sure. That's why eventually base class approach was chosen.

[jansvoboda11]

In D120334#3338314, @yurai007 wrote:

In D120334#3337961, @jansvoboda11 wrote:

The performance implications are pretty interesting! Have you tried avoiding the copy altogether by just storing the reference?

Yes, actually that was my first attempt but I failed miserably. LangOpts::LineComment is mutated in Lexer so LangOpts cannot be simply const&. Making it non-const reference is technically doable with some extra adjustment for Lexer constructors.
However I'm worried a bit how it would impact external LangOpts users after mutating LineComment member by Lexer. As long as LangOpts acts as simple cache it's easy to reason about but when we start to share it with Lexer callers I'm not longer sure. That's why eventually base class approach was chosen.

Got it. It seems like getting rid of the LineComment mutation would be pretty straightforward though. I don't expect any big perf win by replacing copy of a handful of bitfields with indirection, but it would be interesting to see the experiment nonetheless.

[yurai007]

In D120334#3338367, @jansvoboda11 wrote:

In D120334#3338314, @yurai007 wrote:

In D120334#3337961, @jansvoboda11 wrote:

The performance implications are pretty interesting! Have you tried avoiding the copy altogether by just storing the reference?

Yes, actually that was my first attempt but I failed miserably. LangOpts::LineComment is mutated in Lexer so LangOpts cannot be simply const&. Making it non-const reference is technically doable with some extra adjustment for Lexer constructors.
However I'm worried a bit how it would impact external LangOpts users after mutating LineComment member by Lexer. As long as LangOpts acts as simple cache it's easy to reason about but when we start to share it with Lexer callers I'm not longer sure. That's why eventually base class approach was chosen.

Got it. It seems like getting rid of the LineComment mutation would be pretty straightforward though. I don't expect any big perf win by replacing copy of a handful of bitfields with indirection, but it would be interesting to see the experiment nonetheless.

Ok. I can explore more this direction and come back with results.

[serge-sans-paille]

A small local test hints that using a `SmallVector instead of a std::vector` also fixes the assembly bloat. I don't know if it's worth considering as an alternative or as an extra step.

[serge-sans-paille]

https://llvm-compile-time-tracker.com/compare.php?from=79c9072dc009693477242bc1347a2a6c3e419423&to=702bdad5179ebb90c8d8a16c800ffd1b3c44744d&stat=instructions << indeed a lot of green there!

[yurai007]

In D120334#3339764, @serge-sans-paille wrote:

A small local test hints that using a `SmallVector instead of a std::vector` also fixes the assembly bloat. I don't know if it's worth considering as an alternative or as an extra step.

Ok, I'm gonna try SmallVectors on top of this change. LangOptions are widely used so maybe it could bring some extra gains in places where vectors are indeed filled with more data.

[yurai007]

Use simpler approach with const LangOptions reference + rebase.

[cor3ntin]

I like this approach. it seems a lot easier to maintain as people don't have to remember to use LangOptionsBase to not degrade performance

clang/include/clang/Lex/Lexer.h

94

Should we add a comment to explain why this is a reference?

[nikic]

Looks reasonable to me.

[jansvoboda11]

LGTM too if the performance remains the same as with the original revision.

[yurai007]

Unfortunately after change there is lifetime issue in FormatTokenLexer because LangOpts received from getFormattingLangOpts has automatic storage duration and Lexer gets reference to it.
Fix issue and poke CI to make sure clang-tools-extra tests on CI are passing. For now it's only about making CI happy, I'm gonna check also if nice performance gains are preserved (I guess they should be).

[yurai007]

In D120334#3340639, @cor3ntin wrote:

I like this approach. it seems a lot easier to maintain as people don't have to remember to use LangOptionsBase to not degrade performance

I like it either but we need to be careful about LangOptions lifetime as my previous comment shows.

[yurai007]

clang/include/clang/Lex/Lexer.h

94

Good idea. It will be added.

[yurai007]

All comments were addressed. Since broken libarcher/libomp/libomptarget UT are seen on others patches too I think it has nothing to do with this particular change. There is no speed degradation in comparison to LangOptionsBase approach so I believe now change is mature enough to submit.

clang/include/clang/Lex/Lexer.h

 //===- Lexer.h - C Language Family Lexer ------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the Lexer interface.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LEX_LEXER_H
 #define LLVM_CLANG_LEX_LEXER_H
 
-#include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Lex/PreprocessorLexer.h"
 #include "clang/Lex/Token.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include <cassert>
 #include <cstdint>
 #include <string>
 
 namespace llvm {
 
 class MemoryBufferRef;
 
 } // namespace llvm
 
 namespace clang {
 
 class DiagnosticBuilder;
 class Preprocessor;
 class SourceManager;
+class LangOptions;
 
 /// ConflictMarkerKind - Kinds of conflict marker which the lexer might be
 /// recovering from.
 enum ConflictMarkerKind {
   /// Not within a conflict marker.
   CMK_None,
 
   /// A normal or diff3 conflict marker, initiated by at least 7 "<"s,
class Lexer : public PreprocessorLexer {
   const char *BufferStart;
 
   // End of the buffer.
   const char *BufferEnd;
 
   // Location for start of file.
   SourceLocation FileLoc;
 
-  // LangOpts enabled by this language (cache).
-  LangOptions LangOpts;
+  // LangOpts enabled by this language.
+  // Storing LangOptions as reference here is important from performance point
+  // of view. Lack of reference means that LangOptions copy constructor would be
+  // called by Lexer(..., const LangOptions &LangOpts,...). Given that local
+  // Lexer objects are created thousands times (in Lexer::getRawToken,
+  // Preprocessor::EnterSourceFile and other places) during single module
+  // processing in frontend it would make std::vector<std::string> copy
+  // constructors surprisingly hot.
+  const LangOptions &LangOpts;
+
+  // True if '//' line comments are enabled.
+  bool LineComment;
 
   // True if lexer for _Pragma handling.
   bool Is_PragmaLexer;
 
   //===--------------------------------------------------------------------===//
   // Context-specific lexing flags set by the preprocessor.
   //
 

clang/lib/Format/FormatTokenLexer.h

 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LIB_FORMAT_FORMATTOKENLEXER_H
 #define LLVM_CLANG_LIB_FORMAT_FORMATTOKENLEXER_H
 
 #include "Encoding.h"
 #include "FormatToken.h"
+#include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Format/Format.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Regex.h"
 
 #include <stack>
private:
   FormatToken *getNextToken();
 
   FormatToken *FormatTok;
   bool IsFirstToken;
   std::stack<LexerState> StateStack;
   unsigned Column;
   unsigned TrailingWhitespace;
   std::unique_ptr<Lexer> Lex;
+  LangOptions LangOpts;
   const SourceManager &SourceMgr;
   FileID ID;
   const FormatStyle &Style;
   IdentifierTable &IdentTable;
   AdditionalKeywords Keywords;
   encoding::Encoding Encoding;
   llvm::SpecificBumpPtrAllocator<FormatToken> &Allocator;
   // Index (in 'Tokens') of the last token that starts a new line.

clang/lib/Format/FormatTokenLexer.cpp

 namespace format {
 
 FormatTokenLexer::FormatTokenLexer(
     const SourceManager &SourceMgr, FileID ID, unsigned Column,
     const FormatStyle &Style, encoding::Encoding Encoding,
     llvm::SpecificBumpPtrAllocator<FormatToken> &Allocator,
     IdentifierTable &IdentTable)
     : FormatTok(nullptr), IsFirstToken(true), StateStack({LexerState::NORMAL}),
-      Column(Column), TrailingWhitespace(0), SourceMgr(SourceMgr), ID(ID),
+      Column(Column), TrailingWhitespace(0),
+      LangOpts(getFormattingLangOpts(Style)), SourceMgr(SourceMgr), ID(ID),
       Style(Style), IdentTable(IdentTable), Keywords(IdentTable),
       Encoding(Encoding), Allocator(Allocator), FirstInLineIndex(0),
       FormattingDisabled(false), MacroBlockBeginRegex(Style.MacroBlockBegin),
       MacroBlockEndRegex(Style.MacroBlockEnd) {
-  Lex.reset(new Lexer(ID, SourceMgr.getBufferOrFake(ID), SourceMgr,
-                      getFormattingLangOpts(Style)));
+  Lex.reset(new Lexer(ID, SourceMgr.getBufferOrFake(ID), SourceMgr, LangOpts));
   Lex->SetKeepWhitespaceMode(true);
 
   for (const std::string &ForEachMacro : Style.ForEachMacros) {
     auto Identifier = &IdentTable.get(ForEachMacro);
     Macros.insert({Identifier, TT_ForEachMacro});
   }
   for (const std::string &IfMacro : Style.IfMacros) {
     auto Identifier = &IdentTable.get(IfMacro);
void FormatTokenLexer::readRawToken(FormatToken &Tok) {
 
   if (Tok.is(tok::comment) && (Tok.TokenText == "// clang-format off" ||
                                Tok.TokenText == "/* clang-format off */"))
     FormattingDisabled = true;
 }
 
 void FormatTokenLexer::resetLexer(unsigned Offset) {
   StringRef Buffer = SourceMgr.getBufferData(ID);
-  Lex.reset(new Lexer(SourceMgr.getLocForStartOfFile(ID),
-                      getFormattingLangOpts(Style), Buffer.begin(),
-                      Buffer.begin() + Offset, Buffer.end()));
+  LangOpts = getFormattingLangOpts(Style);
+  Lex.reset(new Lexer(SourceMgr.getLocForStartOfFile(ID), LangOpts,
+                      Buffer.begin(), Buffer.begin() + Offset, Buffer.end()));
   Lex->SetKeepWhitespaceMode(true);
   TrailingWhitespace = 0;
 }
 
 } // namespace format
 } // namespace clang

clang/lib/Lex/Lexer.cpp

 /// Lexer constructor - Create a new lexer object for the specified buffer
 /// with the specified preprocessor managing the lexing process.  This lexer
 /// assumes that the associated file buffer and Preprocessor objects will
 /// outlive it, so it doesn't take ownership of either of them.
 Lexer::Lexer(FileID FID, const llvm::MemoryBufferRef &InputFile,
              Preprocessor &PP, bool IsFirstIncludeOfFile)
     : PreprocessorLexer(&PP, FID),
       FileLoc(PP.getSourceManager().getLocForStartOfFile(FID)),
-      LangOpts(PP.getLangOpts()), IsFirstTimeLexingFile(IsFirstIncludeOfFile) {
+      LangOpts(PP.getLangOpts()), LineComment(LangOpts.LineComment),
+      IsFirstTimeLexingFile(IsFirstIncludeOfFile) {
   InitLexer(InputFile.getBufferStart(), InputFile.getBufferStart(),
             InputFile.getBufferEnd());
 
   resetExtendedTokenMode();
 }
 
 /// Lexer constructor - Create a new raw lexer object.  This object is only
 /// suitable for calls to 'LexFromRawLexer'.  This lexer assumes that the text
 /// range will outlive it, so it doesn't take ownership of it.
 Lexer::Lexer(SourceLocation fileloc, const LangOptions &langOpts,
              const char *BufStart, const char *BufPtr, const char *BufEnd,
              bool IsFirstIncludeOfFile)
-    : FileLoc(fileloc), LangOpts(langOpts),
+    : FileLoc(fileloc), LangOpts(langOpts), LineComment(LangOpts.LineComment),
       IsFirstTimeLexingFile(IsFirstIncludeOfFile) {
   InitLexer(BufStart, BufPtr, BufEnd);
 
   // We *are* in raw mode.
   LexingRawMode = true;
 }
 
 /// Lexer constructor - Create a new raw lexer object.  This object is only

 /// return.
 ///
 /// If we're in KeepCommentMode or any CommentHandler has inserted
 /// some tokens, this will store the first token and return true.
 bool Lexer::SkipLineComment(Token &Result, const char *CurPtr,
                             bool &TokAtPhysicalStartOfLine) {
   // If Line comments aren't explicitly enabled for this language, emit an
   // extension warning.
-  if (!LangOpts.LineComment) {
+  if (!LineComment) {
     if (!isLexingRawMode()) // There's no PP in raw mode, so can't emit diags.
       Diag(BufferPtr, diag::ext_line_comment);
 
     // Mark them enabled so we only emit one warning for this translation
     // unit.
-    LangOpts.LineComment = true;
+    LineComment = true;
   }
 
   // Scan over the body of the comment.  The common case, when scanning, is that
   // the comment contains normal ascii characters with nothing interesting in
   // them.  As such, optimize for this case with the inner loop.
   //
   // This loop terminates with CurPtr pointing at the newline (or end of buffer)
   // character that ends the line comment.
if (SkipWhitespace(Result, CurPtr, TokAtPhysicalStartOfLine))
       return true; // KeepWhitespaceMode
 
   SkipIgnoredUnits:
     CurPtr = BufferPtr;
 
     // If the next token is obviously a // or /* */ comment, skip it efficiently
     // too (without going through the big switch stmt).
     if (CurPtr[0] == '/' && CurPtr[1] == '/' && !inKeepCommentMode() &&
-        LangOpts.LineComment &&
-        (LangOpts.CPlusPlus || !LangOpts.TraditionalCPP)) {
+        LineComment && (LangOpts.CPlusPlus || !LangOpts.TraditionalCPP)) {
       if (SkipLineComment(Result, CurPtr+2, TokAtPhysicalStartOfLine))
         return true; // There is a token to return.
       goto SkipIgnoredUnits;
     } else if (CurPtr[0] == '/' && CurPtr[1] == '*' && !inKeepCommentMode()) {
       if (SkipBlockComment(Result, CurPtr+2, TokAtPhysicalStartOfLine))
         return true; // There is a token to return.
       goto SkipIgnoredUnits;
     } else if (isHorizontalWhitespace(*CurPtr)) {
if (Char == '/') {         // Line comment.
       // Even if Line comments are disabled (e.g. in C89 mode), we generally
       // want to lex this as a comment.  There is one problem with this though,
       // that in one particular corner case, this can change the behavior of the
       // resultant program.  For example, In  "foo //**/ bar", C89 would lex
       // this as "foo / bar" and languages with Line comments would lex it as
       // "foo".  Check to see if the character after the second slash is a '*'.
       // If so, we will lex that as a "/" instead of the start of a comment.
       // However, we never do this if we are just preprocessing.
-      bool TreatAsComment = LangOpts.LineComment &&
-                            (LangOpts.CPlusPlus || !LangOpts.TraditionalCPP);
+      bool TreatAsComment =
+          LineComment && (LangOpts.CPlusPlus || !LangOpts.TraditionalCPP);
       if (!TreatAsComment)
         if (!(PP && PP->isPreprocessedOutput()))
           TreatAsComment = getCharAndSize(CurPtr+SizeTmp, SizeTmp2) != '*';
 
       if (TreatAsComment) {
         if (SkipLineComment(Result, ConsumeChar(CurPtr, SizeTmp, Result),
                             TokAtPhysicalStartOfLine))
           return true; // There is a token to return.