kbobyrev added inline comments.
================
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:192
+/// Reads variable length DocID from the buffer and updates the buffer size. If
+/// the stream is terminated, return None.
+llvm::Optional<DocID> readVByte(llvm::ArrayRef<uint8_t> &Bytes) {
----------------
sammccall wrote:
> nit: if the stream is terminated, **consumes all bytes and** returns None.
As discussed offline, when the stream is terminated (i.e. `0` byte indicates
the end of the stream) it just returns `llvm::None`.
================
Comment at: clang-tools-extra/clangd/index/dex/PostingList.h:41
+/// decompressed upon request.
+struct Chunk {
+ // Keep sizeof(Chunk) == 32.
----------------
sammccall wrote:
> sammccall wrote:
> > sammccall wrote:
> > > With the current implementation, this doesn't need to be in the header.
> > > (the layout of vector<chunk> doesn't depend on chunk, you should just
> > > need to out-line the default destructor)
> > >
> > > (using SmallVector<Chunk, 1> or maybe 2 *might* be a win. I'd expect not
> > > though. I'd either stick with std::vector, or measure)
> > (meta-nit: please don't mark comments as done if they're not done - rather
> > explain why you didn't do them!)
> > (using SmallVector<Chunk, 1> or maybe 2 *might* be a win. I'd expect not
> > though. I'd either stick with std::vector, or measure)
>
> You changed this to SmallVector - what were the measurements?
> (SmallVector is going to be bigger than Vector whenever you overrun, so it's
> worth checking)
| Storage type | Memory consumption, MB |
| ----- | ----- |
| `llvm::SmallVector<Chunk, 1>` | 23.7 |
| `llvm::SmallVector<Chunk, 2>` | 25.2 |
| `std::vector<Chunk>` | 23.5 |
It seems like `std::vector<Chunk>` would be the best option here, falling back
to that.
As discussed offline, moving `Chunk` to header seems tedious because of the
default constructor/destructors failures due to incomplete `Chunk` type.
https://reviews.llvm.org/D52300
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
