[PATCH] D137918: [nfc] move expensive parts of Hashing.h into cpp file

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/Hashing.h:51
 #include "llvm/Support/type_traits.h"
 #include <algorithm>
 #include <cassert>
----------------
Trass3r wrote:
> dblaikie wrote:
> > Trass3r wrote:
> > > dblaikie wrote:
> > > > Trass3r wrote:
> > > > > Trass3r wrote:
> > > > > > It'd be good to get rid of this completely since it shows up at the top of the expensive includes list but there's a non-trivial usage left below.
> > > > > https://github.com/Trass3r/llvm-project/actions/runs/3458979320/jobs/5773908563#check-step-6
> > > > > ```
> > > > > *** Expensive headers:
> > > > > 1461737 ms: lvm/include/llvm/ADT/Hashing.h (included 5485 times, avg 266 ms), included via:
> > > > > 1269781 ms: /usr/include/c++/11/algorithm (included 5535 times, avg 229 ms), included via:
> > > > > 987320 ms: /usr/include/c++/11/pstl/glue_algorithm_defs.h (included 5535 times, avg 178 ms), included via:
> > > > > ```
> > > > Do you have stats on how much this patch changes those numbers? 
> > > As mentioned since I couldn't remove the second rotate usage it doesn't change the top includes.
> > > I'm pretty sure I only touched hash_state cause it showed up in the top class parsing costs but now the impact is smaller. Maybe I looked at the numbers only for LLVMSupport back then.
> > Do you think this change is still worth doing?
> I guess the question is more generally whether these non-templated functions were implemented in the header for some good reason (performance etc) or it simply grew over time.
Probably important that these functions get inlined - but we have ThinLTO and such these days for anyone building Clang for speed anyway. I don't have /super/ strong feelings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137918