Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb][Type Completion] Load C++ methods lazily from DWARF #8579

Conversation

Michael137
Copy link

@Michael137 Michael137 commented Apr 16, 2024

Note, this is a no-op when the LLDB setting
plugin.typesystem.clang.experimental-redecl-completion is not set (which is currently the default). So this patch has no effect unless the user explicitly opts into it.

The type-completion rework (aka redecl-completion) implemented in #8222 comes with a large performance penalty, since we now eagerly complete RecordTypes. Completing a RecordType previously unconditionally resolved all member functions of said type. With redecl-completion completion, however, this meant we were now pulling in many more definitions than needed. Without redecl-completion, this isn't a problem, since importing method parameters is cheap (they are imported minimally), so we wouldn't notice that we always resolved all member functions.

This patch tries to load methods lazily when in redecl-completion mode. We do this by introducing a new ExternalASTSource::FindExternalVisibleMethods API which Clang calls when parsing a member access expression. The callback into LLDB will do a FindFunctions call for all methods with the method name we're looking for, filters out any mismatches, and lets Clang continue with its parsing. We still load following methods eagerly:

  1. virtual functions: currently overrides are resolved in CompleteRecordType
  2. operators: currently I couldn't find a point at which Clang can call into LLDB here to facilitate lazy loading
  3. ctors/dtors: same reason as (2)

In our benchmark harness, we saw this patch bring down redecl-completion expression evaluation performance on-par with top-of-tree expression evaluation.

@Michael137
Copy link
Author

@swift-ci test

@Michael137 Michael137 force-pushed the lldb/type-completion-rework/lazy-method-loading branch 2 times, most recently from f262b1f to 073b109 Compare April 16, 2024 14:56
@Michael137
Copy link
Author

@swift-ci test

TypeSystemClang &ts) {
auto CDC1 = candidate_decl_ctx.GetTypeSystem()->DeclContextGetCompilerContext(
candidate_decl_ctx.GetOpaqueDeclContext());
assert(CDC1.size() > 1); // Member functions have at least 2 entries (1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: If a comment is > 1 line, I'd rather put it on top.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

auto CDC1 = candidate_decl_ctx.GetTypeSystem()->DeclContextGetCompilerContext(
candidate_decl_ctx.GetOpaqueDeclContext());
assert(CDC1.size() > 1); // Member functions have at least 2 entries (1
// for method name, 1 for partent class)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parent

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

function_options, sc_list);

auto num_matches = sc_list.GetSize();
if (num_matches > 0) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (num_matches > 0) {
if (!num_matches)
return;```

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

DW_AT_virtuality, DW_VIRTUALITY_none) > DW_VIRTUALITY_none;
const bool is_operator = mem_name.GetStringRef().starts_with("operator");

if (is_ctor || is_operator || is_virtual_method)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you comment here, why only these three in partiuclar?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Michael137 Michael137 force-pushed the lldb/type-completion-rework/lazy-method-loading branch from 073b109 to e56bbcc Compare April 18, 2024 11:30
@Michael137
Copy link
Author

@swift-ci test

@Michael137
Copy link
Author

@swift-ci test Windows

Note, this is a no-op when the LLDB setting
`plugin.typesystem.clang.experimental-redecl-completion` is
not set (which is currently the default). So this patch has
no affect unless the user explicitly opts into it.

The type-completion rework (aka redecl-completion) implemented in
swiftlang#8222 comes with
a large performance penalty, since we now eagerly complete
`RecordType`s. Completing a `RecordType` previously unconditionally
resolved all member function of said type. With redecl-completion
completion, however, this meant we were now pulling in many
more definitions than needed. Without redecl-completion, this
isn't a problem, since importing method parameters is cheap
(they are imported minimally), so we wouldn't notice that
we always resolved all member functions.

This patch tries to load methods lazily when in redecl-completion
mode. We do this by introducing a new `ExternalASTSource::FindExternalVisibleMethods`
API which Clang parses a member access expression. The callback
into LLDB will do a `FindFunctions` call for all methods with the
method name we're looking for, filters out any mismatches, and
lets Clang continue with it's parsing. We still load following
methods eagerly:
1. virtual functions: currently overrides are resolved in `CompleteRecordType`
2. operators: currently I couldn't find a point at which Clang can call
              into LLDB here to facilitate lazy loading
3. ctors/dtors: same reason as (2)

In our benchmark harness, we saw this patch bring down redecl-completion
expression evaluation on-par with top-of-tree expression evaluation.
@Michael137 Michael137 force-pushed the lldb/type-completion-rework/lazy-method-loading branch from bb452ec to 4397f63 Compare April 18, 2024 16:50
@Michael137
Copy link
Author

@swift-ci test

@Michael137
Copy link
Author

@swift-ci test Windows

1 similar comment
@Michael137
Copy link
Author

@swift-ci test Windows

@Michael137 Michael137 merged commit afa82df into swiftlang:stable/20230725 Apr 18, 2024
3 checks passed
Michael137 added a commit to Michael137/llvm-project that referenced this pull request May 3, 2024
Michael137 added a commit to Michael137/llvm-project that referenced this pull request May 3, 2024
Michael137 added a commit to Michael137/llvm-project that referenced this pull request May 6, 2024
Expected failures due to lazy method loading
(swiftlang#8579)

(cherry picked from commit 9507b8b)
Michael137 added a commit to Michael137/llvm-project that referenced this pull request May 8, 2024
Expected failures due to lazy method loading
(swiftlang#8579)

(cherry picked from commit 9507b8b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants