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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
