DO NOT MERGE: Identify places that need reserve.

[kazutakahirata]

No description provided.

[kazutakahirata]

@kuhar I understand that you are working on improving the performance of llvm::append_range.

I did some experiment with this PR:

• I taught llvm::append_range to:
  • automatically call Container::reserve if the given range is a pointer or std::random_access_iterator_tag, and
  • crash on capacity changes after calling Container::insert.
• I built a debug version of compiler (for clean stack traces).
• I used the debug version of the compiler to build an optimized version of clang while manually adding reserve to fix crashes.

One noteworthy point is that I only needed to add a dozen or so calls to reserve. Some ranges are of sets like SmallPtrSet, so size() is immediately available. We might even upstream those calls. Others require linear walk, so we shouldn't upstream those.

I'm wondering if porting those reserve() calls on SmallPtrSet and such to your patch (#136365) might improve the performance. Also, I am wondering if your patch needs to deal with pointer ranges like char *, which I assume doesn't have an iterator category. Note that ArrayRef uses pointers as iterator types.

We could detect the presence of size() on ranges and use that as a hint when given ranges are neither pointer nor std::random_access_iterator_tag. Alternatively, we could manually add calls to reserve() just before append_range where ranges come with size(), which we could detect with some temporary template meta-programming hacks to append_range.

[kuhar]

I understand that you are working on improving the performance of llvm::append_range.

I'm interested and attempted to, but I have very few cycles these days (mostly weekends/holidays). The biggest blocker for me is setting up something I can quickly test locally. @nikic provided some instructions but I haven't had time to try that. Ideally, I'd like a large bitcode file I can wget and then a script to run perf for me and give me the numbers back... I've done it before, using gllvm, and have a whole-program .bc for clang, but that bitcode is very old (from 5.0 era?) and probably not useful anymore.

I thought of a similar approach but slightly tweaked -- I left inline comments.

I'm wondering if porting those reserve() calls on SmallPtrSet and such to your patch (#136365) might improve the performance. Also, I am wondering if your patch needs to deal with pointer ranges like char *, which I assume doesn't have an iterator category. Note that ArrayRef uses pointers as iterator types.

Iterator traits/categories should already handle raw pointers -- I don't think there's anything to do here.

The way I'd approach it would be deciding what compile time checks we need:

1. has_reserve (which you already have) -- otherwise we can't do much
2. has_fast_size (I'd rely on either llvm::size or .size() being present, with the assumption that .size() is fast)
3. has_capacity (like you have) -- for debugging/testing only

Then the remaining exercise would be adding .reserve to containers that can benefit from it and potentially removing/renaming .size() from types where it involves a slow query.

[kuhar]

iterator_traits should already handle plain pointers, so I think we'd want to use that instead of looking for iterator_category?

[kuhar]

This is a cool idea for verifying that it reserved as intended!

[kuhar]

The alternative would be to check that the type either has .size() (which we assume to be fast) or that llvm::size is detected (which already requires random access iterators)