[Lldb-commits] [PATCH] D62477: DWARFASTParserClang: Move attribute parsing into a single function

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added a comment.

In D62477#1519318 <https://reviews.llvm.org/D62477#1519318>, @clayborg wrote:

> Would be good to verify performance doesn't regress from this somehow. Maybe having a large DWARF file with many types and accessing each type and making sure it is completed via the external AST.


I tried to measure this by calling ParseTypes <https://github.com/llvm-mirror/lldb/blob/45e4c0f04232e669f8ece0f1887d28d79b265290/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp#L3003> for each compile unit in a loop. If anything, the readings seem to indicate that the new version is faster (which is possible because we now don't iterate the attribute list twice to search for DW_AT_signature). However, the readings were pretty inconsistent even for identical runs, so this is most likely just noise.

TBH, I'd be surprised if this affects the performance in any way because there is so much other work being done while parsing/completing a type, and this only changes how we iterate over the attribute list. And the complexity of the iteration is still bound by the actual number of attirbutes, the only difference now is that the contained switch will have ~40 case labels instead of some slightly smaller number.



================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:248
+  clang::StorageClass storage = clang::SC_None;
+  const char *mangled_name = nullptr;
+  ConstString name;
----------------
clayborg wrote:
> Why is this stored as a "const char *" and "name" below as a ConstString?
It's stored in the form in which the code expects to use it. Name used both as ConstString and as a c string, so I chose the first. The mangled name is only used as a c string, so I did not want to const-ify it needlessly (though the string will probably end up in the string pool anyway at some point.


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

https://reviews.llvm.org/D62477