[PATCH] D52300: [clangd] Implement VByte PostingList compression

[Kirill Bobyrev via Phabricator via cfe-commits]

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