Page MenuHomePhabricator

Adding caching to libc++ std::function formatter for lookups that require scanning symbols

Authored by shafik on Sep 3 2019, 10:30 AM.



Performance issues lead to the libc++ std::function formatter to be disabled, see D65666

This change is the first of two changes that should address the performance issues and allow us to enable the formatter again.

In some cases we end up scanning the symbol table for the callable wrapped by std::function for those cases we will now cache the results and used the cache in subsequent look-ups. This still leaves a large cost for the initial lookup which will be addressed in the next change.

We are also expanding the tests but they will remain disabled.

Diff Detail

Event Timeline

shafik created this revision.Sep 3 2019, 10:30 AM
friss added a subscriber: friss.Sep 3 2019, 10:53 AM
friss added inline comments.
44–46 ↗(On Diff #218481)

Can we find a better way to test the cache lookups?

Ideally, we'd test that the result actually come from the cache. Maybe we can add some logging to FindSymbolsMatchingRegExAndType and check that it doesn't appear on the second search? Also, to make this look like less of a copy-pasto, I would put the test in a helper function and run that function twice, the second time in a mode that checks that the lookups use the caching.

87–89 ↗(On Diff #218481)


shafik updated this revision to Diff 218797.Sep 4 2019, 3:22 PM
shafik marked 2 inline comments as done.

Addressing comments

  • Using log timers to verify whether we are using the cache or not
  • Switched to llvm::StringMap
friss added inline comments.Sep 4 2019, 3:40 PM
49–59 ↗(On Diff #218797)

Can you wrap all of the above in a helper function? You repeat this same pattern for every variable, the test would be much more readable. It also gives you a good spot to expand a little bit on way the testing involves "timers".

shafik updated this revision to Diff 219604.Sep 10 2019, 2:13 PM
shafik marked an inline comment as done.
  • Refactored test to use a function to do repetitive task
aprantl added inline comments.Sep 10 2019, 3:30 PM
248 ↗(On Diff #219604)

This is performing the lookup twice.

auto it = CallableLookupCache.find(func_to_match);
if (it != CallableLookupCache.end)
  return *it;
272 ↗(On Diff #219604)

Factor these conditions out into a helper function with a descriptive name since it's also used above?

91 ↗(On Diff #219604)

Since this is used in only one place, I would skip the using statement, but that's more a personal style preference.

friss added inline comments.Sep 10 2019, 3:32 PM
31 ↗(On Diff #219604)

Does this negative lookahead regex actually fail if FindSymbolsMatchingRegExAndType is in the output? I would expect the .* afterwards to match basically anything, including what you have in your negative lookahead or another line of the output.

shafik updated this revision to Diff 219621.Sep 10 2019, 4:04 PM
shafik marked 2 inline comments as done.
  • Fixing test based on comments
31 ↗(On Diff #219604)

Good catch, I was trying to be too clever and forgot how it worked.

This is more easily done using matching=False

shafik updated this revision to Diff 219633.Sep 10 2019, 4:52 PM
shafik marked 3 inline comments as done.

Changes based on comments:

  • Removed extra lookup
  • Refactored redundant code
friss accepted this revision.Sep 11 2019, 8:18 AM

This LGTM, in principle, but it's weird to add the testing in a skipped test. I trust that it works on your machine, but we won't know if it works on all the tested configurations. Can committing this wait for the perf issue to be fixed? Do you know how much work this is going to be?

This revision is now accepted and ready to land.Sep 11 2019, 8:18 AM
aprantl added inline comments.Sep 11 2019, 9:34 AM
196 ↗(On Diff #219633)

StringRefs are meant to be value types. A StringRef is only a ptr+offset so we are not being much more efficient by passing a pointer to a StringRef to a function. This should just be a StringRef value argument.

275 ↗(On Diff #219633)

Not your code, but since you're there: symbol &&

Actually, this would really benefit from another descriptive helper methid since this is used in several places and not obvious to me what is being checked here. Lambda invokes, block invokes, both?

Otherwise this LGTM now.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2019, 4:06 PM