[PATCH] D50261: [AST] Remove unnecessary indirections in DeclarationNameTable

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

On the face, this seems to be a nice patch.  Removing extra allocations and replacing them with the stack is a good thing IMO. This is clearly an example of PIMPL, which while it has its advantages, performance is typically a hit.  My 2 concerns with this are:

1- The increased size of DeclarationNameTable will cause a performance regression in the case where these folding sets are used rarely.  Copies (which are obviously deleted, unless memcpy'ed somewhere)/moves of DeclarationNameTable are now more expensive.  Based on your analysis, it seems that this is a 'net win', but I'd like to see if we can disallow 'move' as well.

2- The advantage of PIMPL is a reduction in compile time.  Now, every file that uses DeclarationName.h (an extensive list of .cpp files, but more importantly, Decl.h, DeclBase.h, and Sema.h).  Could you perhaps do a clean build both before/after this change with 'time' (http://man7.org/linux/man-pages/man1/time.1.html) and show the results?  I hope it ends up being a small enough change to be acceptable, but it would be nice to have an informed decision here.



================
Comment at: include/clang/AST/DeclarationName.h:450
   DeclarationNameTable &operator=(const DeclarationNameTable &) = delete;
 
   /// getIdentifier - Create a declaration name that is a simple
----------------
DeclarationNameTable should definitely have the move operators dealt with here.  I suspect/hope the answer is 'deleted', but I don't think it matters generally.

Additionally, I'd like to see the destructor line left and set to 'default', since those 2 would complete the 'rule of 5'.


Repository:
  rC Clang

https://reviews.llvm.org/D50261