[PATCH] D50214: Add inherited attributes before parsed attributes.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/AST/DeclBase.cpp:854-859
+  auto I = Attrs.begin(), E = Attrs.end();
+  for (; I != E; ++I) {
+    if (!(*I)->isInherited())
+      break;
+  }
+  Attrs.insert(I, A);
----------------
Meinersbur wrote:
> aaron.ballman wrote:
> > The unfortunate part about this is that inherited attributes are fairly common, so this extra searching may happen more often than we'd like. I wonder how bad it would be to keep track of the location within the list where the inherited attributes start. Then again, the list of attributes should be relatively short in most cases, so this search isn't likely to be too expensive.
> > 
> > Having some performance measurements might help decide this, too.
> Timed clang-compilation using perf (`perf stat bin/clang llvm/unittests/IR/InstructionsTest.cpp ...`), before this patch (r339574):
> ```
>        7647.415800      task-clock (msec)         #    0.983 CPUs utilized
>                289      context-switches          #    0.038 K/sec
>                 26      cpu-migrations            #    0.003 K/sec
>             86,494      page-faults               #    0.011 M/sec
>     19,068,741,863      cycles                    #    2.493 GHz
>     26,581,355,844      instructions              #    1.39  insn per cycle
>      6,242,394,037      branches                  #  816.275 M/sec
>        128,405,656      branch-misses             #    2.06% of all branches
> 
>        7.779848330 seconds time elapsed
> ```
> 
> With this patch:
> ```
>        7679.173467      task-clock (msec)         #    0.987 CPUs utilized
>                321      context-switches          #    0.042 K/sec
>                 23      cpu-migrations            #    0.003 K/sec
>             86,071      page-faults               #    0.011 M/sec
>     19,150,248,538      cycles                    #    2.494 GHz
>     26,667,465,697      instructions              #    1.39  insn per cycle
>      6,262,381,898      branches                  #  815.502 M/sec
>        128,527,669      branch-misses             #    2.05% of all branches
> 
>        7.779477815 seconds time elapsed
> ```
> (Intel(R) Xeon(R) Platinum 8180M CPU @ 2.50GHz)
> 
> The vector insert operation is also be `O(n)`. If the number of non-inherited is expected to be smaller, we can instead search for the first inherited attribute starting at the end of the vector. If we want to avoid the `O(n)` entirely, we need one list for inherited and another for non-inherited attributes.
Thank you for gathering this data -- those perf numbers look reasonable to me. We can refactor for performance later if it ever turns up on the hot path.


Repository:
  rC Clang

https://reviews.llvm.org/D50214