[PATCH] D87326: [ADT] Fix reference invalidation when self referencing a SmallVector

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D87326#2273732 <https://reviews.llvm.org/D87326#2273732>, @njames93 wrote:

> @nikic Would you be able to see what the delta with this is https://gist.github.com/njames93/f26f159f06bda9e7ed2270adb39d9b08, should apply on top of trunk. It has the most expensive part of the grow buffer defined outline to dissuade inlining, may reduce binary size and improve performance with gcc9.3

Unfortunately this makes things even worse, with clang binary size increasing from 80560905 to 85871535 bytes, which is an increase of 6.5%. Max RSS goes up by 3-5% accordingly.

Before looking further into what exactly is going on there, I would like to take a step back and ask whether this change is really necessary. SmallVector is some of the most performance-critical code in the LLVM project and has tens of thousands of use-sites that magnify any change to it. Given what this patch does, I think that at least some degree of either performance of code-size impact is not avoidable (though the exact impact can be mitigated). This also makes the implementation of SmallVector quite a bit more complex, with subtle invariants to upkeep.

The problem this patch is solving seems like something of an edge-case to me, and specifying that "inserting self-references requires up-front reservation of enough capacity" seems like a sufficient solution (possibly combined with some assertions to make this checkable without asan). LLVM's ADT library is thankfully in a position where it does not need to comply with the std::vector spec to the letter, and can deviate from it where useful. For example, the LLVM programming manual mentions that SmallVector forgoes some exception-safety guarantees to achieve better performance. This seems like a similar situation.

I'm sure that if sufficient effort is put into it, it's possible to make this change with a lower impact. I looked at the libc++ implementation a bit, and there are a few differences that stand out, e.g. libc++ more carefully splits the inline fast-path from the out-of-line slow-path for methods like push_back. It also seems like it provides weaker guarantees on some methods, e.g. the `assign()` implementation does not look grow-safe at all. Figuring out what the most efficient way to do this is would take a lot of time though, as it would involve reapplying this patch in pieces, measuring the impact individual parts have, and trying out variations. If it's necessary to do this across two different host compilers that evidently have quite different optimization behavior in this area, this becomes even more involved. So again the question: Is it really necessary?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87326/new/

https://reviews.llvm.org/D87326