This revision was automatically updated to reflect the committed changes.
Closed by commit rL342965: [clangd] Implement VByte PostingList compression
(authored by omtcyfz, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D52300?vs=166849
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 readVByte(llvm::ArrayRef &Bytes) {
-
kbobyrev updated this revision to Diff 166849.
kbobyrev marked 4 inline comments as done.
kbobyrev added a comment.
Address post-LG comments, remove fuzzer.
https://reviews.llvm.org/D52300
Files:
clang-tools-extra/clangd/index/dex/Dex.cpp
clang-tools-extra/clangd/index/dex/PostingList.cpp
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
LG, don't forget about the fuzzer!
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:41
"Posting List iterator can't advance() at the end.");
-
kbobyrev updated this revision to Diff 166843.
kbobyrev marked 13 inline comments as done and an inline comment as not done.
kbobyrev edited the summary of this revision.
kbobyrev added a comment.
Address a round of comments, fallback to `std::vector`.
https://reviews.llvm.org/D52300
Files:
c
sammccall added a comment.
Mostly looks good, a few further simplifications...
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:59
+CurrentID = std::lower_bound(CurrentID, std::end(DecompressedChunk), ID);
+// Return if the position was found in current ch
kbobyrev updated this revision to Diff 166831.
kbobyrev marked 17 inline comments as done.
kbobyrev added a comment.
- Simplify code
- Disallow empty `PostingList`s and update tests
https://reviews.llvm.org/D52300
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/index
sammccall added a comment.
In https://reviews.llvm.org/D52300#1241754, @kbobyrev wrote:
> I addressed the easiest issues. I'll try to implement separate storage
> structure for `Head`s and `Payload`s which would potentially make the
> implementation cleaner and easier to understand (and also mo
kbobyrev added inline comments.
Comment at: clang-tools-extra/clangd/index/dex/PostingList.cpp:29
+ DecompressedChunk = ChunkIndex->decompress();
+ InnerIndex = begin(DecompressedChunk);
+}
sammccall wrote:
> nit: we generally use members (Decompres
lebedev.ri added a comment.
In https://reviews.llvm.org/D52300#1241776, @ioeric wrote:
> In https://reviews.llvm.org/D52300#1241754, @kbobyrev wrote:
>
> > Also, I'll refine https://reviews.llvm.org/D52047 a little bit and I
> > believe that is should be way easier to understand performance + me
ioeric added a comment.
In https://reviews.llvm.org/D52300#1241754, @kbobyrev wrote:
> Also, I'll refine https://reviews.llvm.org/D52047 a little bit and I believe
> that is should be way easier to understand performance + memory consumption
> once we have these benchmarks in. Both @ioeric and
kbobyrev updated this revision to Diff 166453.
kbobyrev marked 9 inline comments as done.
kbobyrev added a comment.
I addressed the easiest issues. I'll try to implement separate storage
structure for `Head`s and `Payload`s which would potentially make the
implementation cleaner and easier to un
kbobyrev added a comment.
@sammccall thank you for the comments, I'll improve it. @ilya-biryukov also
provided valuable feedback and suggested that the code looks complicated now.
We also discussed different compression approach which would require storing
`Head`s and `Payload`s separately so t
sammccall added a comment.
Very nice!
I think the data structures can be slightly tighter, and some of the
implementation could be easier to follow. But this seems like a nice win.
Right-sizing the vectors seems like an important optimization.
Comment at: clang-tools-extra/c
kbobyrev updated this revision to Diff 166278.
kbobyrev added a comment.
- Update unit tests with iterator tree string representation to comply with the
new format
- Don't mark constructor `explicit` (previously it only had one parameter)
- Fix `Limits` explanation comment (`ID > Limits[I]` -> `I
kbobyrev created this revision.
kbobyrev added reviewers: ioeric, sammccall, ilya-biryukov.
kbobyrev added a project: clang-tools-extra.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, mgorny.
kbobyrev edited the summary of this revision.
This patch implements Variable-length Byte
16 matches
Mail list logo
