[PATCH] D31338: Move ParsedAttrInfos into a registry and point to one in ParsedAttr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Basic/Attributes.cpp:101-103
+  for (ParsedAttrInfoRegistry::iterator it = ParsedAttrInfoRegistry::begin(),
+                                        ie = ParsedAttrInfoRegistry::end();
+       it != ie; ++it) {
----------------
john.brawn wrote:
> aaron.ballman wrote:
> > Range-based for loop? Also, `it` and `ie` don't meet the usual naming conventions.
> > 
> > One thing I'm not keen on with this is the performance hit. We spent a decent amount of effort making this lookup fast and it's now a linear search through all of the attributes and requires memory allocations and deallocations to perform the search.
> Yes, I've done some experiments and in the pathological case (lots of trivial functions with the xray_log_args attribute) the slowdown is noticeable. I've done some tinkering and I think the best way to resolve this is to first use a generated function (i.e. something like the current getAttrKind) to look up the attributes that are compiled into clang, then if that fails look in the registry to find a match.
I think that approach makes sense and is something we should probably do up front. It doesn't seem like `llvm::Registry` has a way for us to mark where the plugin attributes start to allow for quickly searching for plugin attributes in the fallback case.

I don't see a particularly good way around the memory allocations, however. Ideally, this could be something that's arena allocated on the `ASTContext` rather than using `unique_ptr`, but I think that would be tricky here because of library layering. After switching the lookup functionality, I'd be curious to know if there is a noticeable performance degredation still, or if that falls out into the noise.


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

https://reviews.llvm.org/D31338