aprantl marked an inline comment as done.
aprantl added inline comments.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp:138-143
} else if (has_tag) {
if (log)
m_module.LogMessage(log, "FindByNameAndTag()");
m_apple_types_up->FindByNameAndTag(type_name.GetStringRef(), tag, offsets);
} else
m_apple_types_up->FindByName(type_name.GetStringRef(), offsets);
----------------
clayborg wrote:
> We can probably move your code above to here:
> ```
> } else {
> if (context.GetSize() > 1) && (context[1].tag == DW_TAG_class_type ||
> context[1].tag == DW_TAG_structure_type)) {
> DIEArray class_matches;
> m_apple_types_up->FindByName(context[1].name, class_matches);
> if (class_matches.empty())
> return;
> }
> if (has_tag) {
> if (log)
> m_module.LogMessage(log, "FindByNameAndTag()");
> m_apple_types_up->FindByNameAndTag(type_name.GetStringRef(), tag,
> offsets);
> } else
> m_apple_types_up->FindByName(type_name.GetStringRef(), offsets);
> }
> ```
>
> Or better yet:
> ```
> } else {
> if (has_tag) {
> if (log)
> m_module.LogMessage(log, "FindByNameAndTag()");
> m_apple_types_up->FindByNameAndTag(type_name.GetStringRef(), tag,
> offsets);
> } else
> m_apple_types_up->FindByName(type_name.GetStringRef(), offsets);
> // If we have too many matches, we might want to ensure that the context
> // that this type is contained in actually exists so we don't end up
> pulling
> // in too much DWARF for common type like "iterator".
> if (offsets.size() > THRESHOLD) {
> if (context.GetSize() > 1) && (context[1].tag == DW_TAG_class_type ||
> context[1].tag ==
> DW_TAG_structure_type)) {
> DIEArray class_matches;
> m_apple_types_up->FindByName(context[1].name, class_matches);
> if (class_matches.empty()) {
> offsets.clear();
> return;
> }
> }
> }
> }
> ```
>
> We might also be able to remove some offsets from "offsets" by iterating
> through "offsets" and removing any values that come before an entry in
> "class_matches". For example if offsets contains:
> ```
> offsets = [0x1100, 0x2100, 0x3100, 0x4100]
> ```
> and
> ```
> class_offsets = [0x4000]
> ```
> could we remove "[0x1100, 0x2100, 0x3100]" from offsets? Maybe not with
> DW_AT_specification and DW_AT_abstract_origin?
Moving the code into the `else` (first suggestion) is equivalent, but may be
more obvious to read. Thanks.
The second suggestions is not a clear win to me. The point of this patch is to
eliminate entire .o files quickly that don't have the parent class type in it,
in order to avoid extracting type DIEs for matches. The assumption here is that
an accelerator table lookup is much cheaper than extracting a DIE. I believe
the threshold is 1.
I also learned to never make assumptions about performance, so here are some
numbers for my clang benchmark:
```
original: 8.7s
this patch: 5.9s
threshold 100: 6.5s
threshold 50: 6.5s
threshold 25: 6.5s
threshold 12: 6.4s
threshold 6: 6.4s
threshold 3: 6.4s
threshold 1: 6.5s
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68678/new/
https://reviews.llvm.org/D68678
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
