[Lldb-commits] [PATCH] D87675: [lldb/DWARF] Refactor to prefer emplace_back() vs. push_back()

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Ok, this is something we can talk about. dwarf parsing is a big bottle neck, so spending some time to optimize that (and even sacrifice some readability/mantainability/etc. if needed) is worthwhile.

That said, I am pretty sure that any performance gains you're seeing (you didn't explain what/how you're measuring so it's hard to say for sure), are due to the additional inlining opportunities afforded by moving this function to the header, and _not_ because of the use of emplace_back. In my comparison <https://godbolt.org/z/5YTrqs>, the two implementations ended up looking fairly different, but I don't think the differences cannot be explained by the use of emplace_back. In both cases, the compiler was able to fully optimize away all the push_backs, constructor calls, etc., and the fast path for both functions consists of a bunch of move instructions. If one of the two versions ends up being slower, then this indicates a missed optimization opportunity on the side of the compiler (which could be removed or even reversed by using a different compiler), and not a fundamental problem with using push_back with trivially copyable structs.

_That_ said, I don't think adding the constructor is necessarily bad in this case, so I think it's fine to stick with the current implementation.


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

https://reviews.llvm.org/D87675