Page MenuHomePhabricator

Fix a regression in DWARF access speed caused by svn revision 356190
ClosedPublic

Authored by clayborg on May 29 2019, 2:32 PM.

Details

Summary

The issue was caused by the error checking code that was added. It was incorrectly adding an extra abbreviation when DWARFEnumState::Complete was received since it would push an extra abbreviation onto the list with the abbreviation code of zero. This cause m_idx_offset in each DWARFAbbreviationDeclarationSet to be set to UINT32_MAX. This valid indicates we must linearly search for attributes, not access them in O(1) time. This caused every DWARFDebugInfoEntry that would try to get its DWARFAbbreviationDeclaration from the CU's DWARFAbbreviationDeclarationSet to always linearly search the abbreviation set for a given abbreviation code. Easy to see why this would cause things to be slow.

This regression was caused by: https://reviews.llvm.org/D59370. I asked to ensure there was no regression is parsing or access speed, but that must not have been done. In my test with 40 DWARF files trying to set a breakpoint by function name and in a header file, I see a 8% speed improvement with this fix.

There was no regression in correctness, just very inefficient access.

Added full unit testing for DWARFAbbreviationDeclarationSet parsing to ensure this doesn't regress.

Diff Detail

Event Timeline

clayborg created this revision.May 29 2019, 2:32 PM
clayborg edited the summary of this revision. (Show Details)May 29 2019, 2:33 PM
aprantl accepted this revision.May 29 2019, 4:22 PM
aprantl added inline comments.
source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
32

Not being too familiar with the API I still find the control flow in this loop to be hard to follow. Is offset_ptr incremented by extract()?

source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
48

FYI I think the proper way to doxygenify this is:

/// Unit test accessor functions.
/// @{
 ...
/// @}
This revision is now accepted and ready to land.May 29 2019, 4:22 PM
clayborg closed this revision.May 30 2019, 8:39 AM
clayborg marked an inline comment as done.

r362105 | gclayton | 2019-05-30 08:32:33 -0700 (Thu, 30 May 2019) | 13 lines

Fix a regression in DWARF access speed caused by svn revision 356190

The issue was caused by the error checking code that was added. It was incorrectly adding an extra abbreviation when DWARFEnumState::Complete was received since it would push an extra abbreviation onto the list with the abbreviation code of zero. This cause m_idx_offset in each DWARFAbbreviationDeclarationSet to be set to UINT32_MAX. This valid indicates we must linearly search for attributes, not access them in O(1) time. This caused every DWARFDebugInfoEntry that would try to get its DWARFAbbreviationDeclaration from the CU's DWARFAbbreviationDeclarationSet to always linearly search the abbreviation set for a given abbreviation code. Easy to see why this would cause things to be slow.

This regression was caused by: https://reviews.llvm.org/D59370. I asked to ensure there was no regression is parsing or access speed, but that must not have been done. In my test with 40 DWARF files trying to set a breakpoint by function name and in a header file, I see a 8% speed improvement with this fix.

There was no regression in correctness, just very inefficient access.

Added full unit testing for DWARFAbbreviationDeclarationSet parsing to ensure this doesn't regress.

Differential Revision: https://reviews.llvm.org/D62630

source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
32

Yes. It is passed to data.GetXXX calls and serves as the thread safe offset into the data.