[PATCH] D51310: [clangd] Implement iterator cost

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

In https://reviews.llvm.org/D51310#1214548, @kbobyrev wrote:

> It's probably better to roll out proximity path boosting & actual two-stage filtering before rolling this out.


Up to you (I don't understand the interaction), but this looks good to go as-is. I'd expect a 10% speedup to be pretty robust to the sorts of changes you're talking about.



================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:90
     sync();
+    std::sort(begin(Children), end(Children),
+              [](const std::unique_ptr<Iterator> &LHS,
----------------
this is a performance heuristic and deserves a brief comment (about *why* we're best to have the shortest first). Not too many details, handwavy is fine :-)


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:93
+                 const std::unique_ptr<Iterator> &RHS) {
+                return LHS->cost() < RHS->cost();
+              });
----------------
(I'd actually suggest declaring operator< in iterator.h and making it compare by cost, it seems natural/important enough, and saves duplicating this lambda)


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:125
+  size_t cost() override {
+    return std::accumulate(
+        begin(Children), end(Children), Children.front()->cost(),
----------------
The use of `std::accumulate` here seems less clear than the direct:
```size_t Smallest = ...;
for (const auto& Child : Children)
  Smallest = std::min(Smallest, Child->cost());
return Smallest;
```
For better or worse, functional styles don't have the most natural syntax in C++, and (IMO) shouldn't be used just because they fit.

(This is consistent with other places, but I also think that the same applies there)


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:125
+  size_t cost() override {
+    return std::accumulate(
+        begin(Children), end(Children), Children.front()->cost(),
----------------
sammccall wrote:
> The use of `std::accumulate` here seems less clear than the direct:
> ```size_t Smallest = ...;
> for (const auto& Child : Children)
>   Smallest = std::min(Smallest, Child->cost());
> return Smallest;
> ```
> For better or worse, functional styles don't have the most natural syntax in C++, and (IMO) shouldn't be used just because they fit.
> 
> (This is consistent with other places, but I also think that the same applies there)
actually, we've sorted by cost, so don't we just want Children.front().cost() here?


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:258
 
+  size_t cost() override {
+    return std::accumulate(
----------------
as above, we've sorted, so just Children.back()->cost()?


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:377
 
+  size_t cost() override { return Limit; }
+
----------------
min() this with Child->cost()?


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:99
+  /// Returns an estimate of advance() calls before the iterator is exhausted.
+  virtual size_t cost() = 0;
 
----------------
const?


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:99
+  /// Returns an estimate of advance() calls before the iterator is exhausted.
+  virtual size_t cost() = 0;
 
----------------
sammccall wrote:
> const?
I know this terminology comes from elsewhere, but I struggle with "cost" as a metaphor because I expect it to aggregate its children as a sum (at least in cases like "or" when all children will be advanced until end).

Maybe `estimateSize()`?


https://reviews.llvm.org/D51310