This is an archive of the discontinued LLVM Phabricator instance.

DWARFASTParserClang: Move attribute parsing into a single function
ClosedPublic

Authored by labath on May 27 2019, 3:25 AM.

Details

Summary

The ParseTypeFromDWARF function consists of a huge switch on the kind of
type being parsed. Each case in this switch starts with parsing the
attributes of the current DIE. A lot of these attributes are specific to
one kind of a type, but a lot of them are common too, leading to code
duplication.

This patch reduces the duplication (and the size of ParseTypeFromDWARF)
by moving the attribute parsing to a separate function. It creates a
struct (ParsedTypeAttributes), which contains a parsed form of all
attributes which are useful for parsing any kind of a type. The parsing
code for a specific type kind can then access the fields which are
relevant for that specific case.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.May 27 2019, 3:25 AM
labath updated this revision to Diff 201497.May 27 2019, 4:19 AM
  • also include DW_AT_signature in the attribute list to increase consistency and avoid iterating over the attribute list twice.

Minor nit where we are storing name a ConstString but mangled name as "const char *". Fix if needed.

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.

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
248 ↗(On Diff #201497)

Why is this stored as a "const char *" and "name" below as a ConstString?

labath marked an inline comment as done.May 29 2019, 4:08 AM

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 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.

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
248 ↗(On Diff #201497)

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.

clayborg accepted this revision.May 29 2019, 3:29 PM

As long as the perf is similar I am good.

This revision is now accepted and ready to land.May 29 2019, 3:29 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 2:38 AM