[Lldb-commits] [PATCH] D68678: Speed up accelerator table lookups

[Adrian Prantl via Phabricator via lldb-commits]

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