[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In general, I think this makes sense. However:

> This change solves this issue nicely. Since we will be only adding code at the back of the buffer, the offsets are always constant even if we grow the buffer many times and all the access to new buffer will be valid. We do add a number of indirections to BufferStart, but performance impact on actual compile time turned out to be negligible. The only visible performance trend seems to be 0.5%~0.7% increase in instruction count.

Can you give some performance numbers for how this impacts compile time performance for some large C and C++ projects? https://llvm-compile-time-tracker.com/ might be of help in gathering that data, FWIW.

In terms of whether to use `unsigned` vs a specific type; I don't have a strong opinion, but it'd be good to at least `static_assert` properties we care about (like the type being at least 32 bits). We've had efforts in the past to allow for > 2GB source files, so I weakly think it would make sense to use a 64-bit value explicitly up front, but I have no idea how that changes the performance characteristics for the changes either.

In terms of whether to expose access to pointers to the underlying buffer through lexer APIs... I sort of agree with @davrec that it would be good to avoid exposing those interfaces given that the pointer values are likely to be invalidated when the buffer grows. My instinct is that Clang developers are unlikely to consider that buffer to be something that can be invalidated, so if we retain an interface to get a pointer to the buffer when you get to the point of actually allowing it to grow, it'd  be nice if we can find some way to forcefully grow the buffer to help catch misuses in Clang when running the llvm-lit tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143142