[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

[Sebastian Pop via cfe-commits]

sebpop added a comment.

In https://reviews.llvm.org/D25624#583163, @mehdi_amini wrote:

> I'd like to understand why only the destructor?


We have committed a patch to inline the constructor of std::string.
Do you think we should inline some other functions?

In https://reviews.llvm.org/D25624#583167, @mehdi_amini wrote:

> I talked with Eric on IRC, he mentioned some benchmarks were ran. I'd like to understand what was the baseline?


We ran a proprietary benchmark compiled with clang + libc++ and compared against clang + libstdc++.
With this patch, libc++ is on par with libstdc++.

> Here we add *both* the inline keyword and the always_inline attribute. I'd like to know if there is a benchmarks that shows that always_inline is beneficial on top of the inline keyword. 
>  If we need to add always_inline anywhere: this is likely an inliner heuristic failure and we should at minima track it as an example to improve it.

This is a good observation: I am not sure we ran the benchmark without the always_inline attribute.
We will try again wihout that attribute and report whether it matters performance wise.
Thanks Mehdi for catching this!


https://reviews.llvm.org/D25624