[Lldb-commits] [lldb] [WIP][lldb] Use forward decls with redeclared definitions instead of minimal import for records (PR #95100)

[Michael Buch via lldb-commits]

Michael137 wrote:

> * when you say "slower", what exactly does that mean. How much slow down are we talking about?
> * the "increased number of DWARF searches", is that due to clang asking for definitions of types more eagerly? If yes, do you have some examples of where are these extra definitions coming from?

For example, attaching LLDB to Clang and evaluating a method on it would take ~35s instead of the baseline ~6s. One of the main contributors to this is that completing a definition for a class would try to resolve all fields *and* member functions of that class (see `DWARFASTParserClang::CompleteRecordType` where it resolves the member function DIEs). This in turn completes the parameters, etc. So we end up trying to pull in many more definitions than needed. The tricky part here is that anything that calls `getDefinition` (which we do in the `ASTImporter` for example) could trigger completion. So we can have situations where the user requests a `GetForwardCompilerType` but that ends up trying to `CompleteRedeclChain`. The current patch breaks the notion of `{Forward/Layout/Full}CompilerType`, but haven't found a good way of fixing that yet. 

Another source of overhead are cases where we try to find the definition DIE using a commonly used name such as `__base`, which we find many matches for in the index, but we fail to find a correct match. Such false positive lookups are quite expensive. I forget the details at the moment but IIRC this patch just makes those lookups more frequent. And the actual failure to choose the correct name for lookup is a consequence of how templates are represented in the LLDB AST.

If we restrict LLDB to only complete fields (but complete methods only as needed), we get much closer to the baseline. Though "lazily" completing methods has other consequences (some features in LLDB rely on all methods being available, e.g., auto-complete, etc.). Apart from that, there still seem to be some redundant global lookups for namespaces and functions that add to the overhead. Once we get closer to the baseline performance I can provide some more accurate benchmark results. Also, happy to hear other suggestions/concerns regarding this.

> * I see one test which tries to skip itself conditionally based on a setting enabling this, but I don't see the code for the handling the setting itself. Is the intention to add the setting, or will this be a flag day change?

Good catch! I cherry-picked these commits from the `apple/llvm-project` fork where we put these changes behind a setting. But upstream I would like to avoid doing so. I'll remove the decorator from the test (we can just XPASS it)

> * I'm intrigued by this line "Instead of creating partially defined records that have fields but possibly no definition, ...". Until last week, I had assumed that types in lldb (and clang) can exist in only one of two states "empty/forward declared/undefined" and "fully defined". I was intrigued to find some support for loading only fields (`setHasLoadedFieldsFromExternalStorage`, et al.) in clang (but not in lldb, AIUI). Does this imply that the code for loading only the fields is broken (is the thing you're trying to remove)?

Good question, I've been having trouble getting small reproducible for this. We've had numerous workarounds around the ASTImporter since the original patch was proposed that addressed these types of issues. E.g., https://reviews.llvm.org/D71378 (which isn't quite the "definition but no fields" situation, but wouldn't be surprised if historically this could've happened in some corner cases of the import mechanism). A common issue I've seen in stacktraces in the past are situtations where we started an `ASTImporter::Import`, and within that called `LoadFieldsFromExternalStorage`, which would then eventually recursively call back into `ASTImporter::Import`, messing up the various bookkeeping structures. I'll try to dig up an example of this

https://github.com/llvm/llvm-project/pull/95100