This is an archive of the discontinued LLVM Phabricator instance.

Re-enable std::function formatter with fixes to improve non-cached lookup performance
ClosedPublic

Authored by shafik on Nov 6 2019, 10:37 AM.

Details

Summary

This PR depends on D67111

Performance issues lead to the libc++ std::function formatter to be disabled. We addressed some of those performance issues by adding caching see D67111

This PR fixes the first lookup performance by not using FindSymbolsMatchingRegExAndType(...) and instead finding the compilation unit the std::function wrapped callable should be in and then searching for the callable directly in the CU.

Diff Detail

Event Timeline

shafik created this revision.Nov 6 2019, 10:37 AM
shafik edited reviewers, added: aprantl; removed: aam.Nov 6 2019, 11:00 AM
friss added a subscriber: friss.Nov 6 2019, 12:30 PM
friss added inline comments.
source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
274 ↗(On Diff #228102)

Why a new variable?

287 ↗(On Diff #228102)

Doesn't matter too much for lambdas, but shouldn't this be "operator()" ?
Is this guaranteed to return a valid index? Should we assert on it?

293 ↗(On Diff #228102)

this is basically is_lambda. As we filter it above, why do we need this test?

source/Symbol/CompileUnit.cpp
101 ↗(On Diff #228102)

This seems pretty expensive. Can we Force the parsing of the functions just in the compiler unit rather than the whole symbol file? If you are using a dSYM, then this is going to load all the debug information in one go.

friss added inline comments.Nov 6 2019, 2:27 PM
source/Symbol/CompileUnit.cpp
101 ↗(On Diff #228102)

Sorry, I somehow missed that this takes a CU as argument. This seems fine.

aprantl added inline comments.Nov 6 2019, 4:45 PM
include/lldb/Symbol/CompileUnit.h
172 ↗(On Diff #228102)

Document what does the bool return value do?

source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
299 ↗(On Diff #228102)

func_sp?

aprantl accepted this revision.Nov 6 2019, 4:46 PM

Seems fine otherwise, I think.

include/lldb/Symbol/CompileUnit.h
174 ↗(On Diff #228102)

perhaps calling it is_match makes this more obvious?

This revision is now accepted and ready to land.Nov 6 2019, 4:46 PM
shafik updated this revision to Diff 228533.Nov 8 2019, 3:33 PM
shafik marked 6 inline comments as done.

After updating branch the stepping test started failing. It was failing because the __invoke case was subtly broken in this patch. Fixing that case opened up a lot of refactoring opportunities. So this patch ends up being a large refactor and in many ways a simplification now.

shafik added a comment.Nov 8 2019, 3:34 PM

@aprantl you will want to take a second look at this, I did some major refactoring.

Why not have the FindFunctions lambda take a FunctionSP? It would be easy to get the Function name out of the function in the lambda, and that would make the function more general (and also match what the Foreach does...

aprantl added inline comments.Nov 11 2019, 1:15 PM
lldb/include/lldb/Symbol/CompileUnit.h
169

This doesn't explain what the bool return does.

shafik marked an inline comment as done.Nov 11 2019, 3:08 PM

Why not have the FindFunctions lambda take a FunctionSP? It would be easy to get the Function name out of the function in the lambda, and that would make the function more general (and also match what the Foreach does...

It was not obvious how they were using it at first, good catch.

shafik updated this revision to Diff 228774.Nov 11 2019, 3:10 PM
  • Adjust comment
  • matching_lambda argument is now FunctionSP to match ForeachFunction signature.
shafik updated this revision to Diff 228923.Nov 12 2019, 11:00 AM

Applying clang-format

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2019, 11:33 AM