This is an archive of the discontinued LLVM Phabricator instance.

Refactor BreakpointResolver::SetSCMatchesByLine()
ClosedPublic

Authored by aprantl on Aug 29 2018, 12:39 PM.

Details

Summary

Refactor BreakpointResolver::SetSCMatchesByLine() to make it easier to
read/understand/maintain.

As a side-effect, this should also improve the performance by avoiding
lots ofcostly vector element removals and switching from a std::map to
a SmallDenseSet.

Diff Detail

Event Timeline

aprantl created this revision.Aug 29 2018, 12:39 PM
shafik added a subscriber: shafik.Aug 29 2018, 2:19 PM

Very nice! Do we have a way of verifying the performance gain?

source/Breakpoint/BreakpointResolver.cpp
183–230

The number 16 and 8 below where do they come from?

184

GetSize() returns uint32_t we should match the type.

214

I don't think you need a capture here.

228

Although in the other lambdas being explicit with the capture list might reduce readability here we are only capturing blocks_with_breakpoints

aprantl updated this revision to Diff 163198.Aug 29 2018, 2:47 PM
aprantl marked 2 inline comments as done.

Address review feedback.

aprantl marked an inline comment as done.Aug 29 2018, 2:49 PM

I don't have any numbers to back up the performance claim. My primary motivation for this patch was to prepare the code for adding extra functionality; the performance gain was only side-effect. One way to measure it would be to open a large project and set a regexp breakpoint on something very common.

source/Breakpoint/BreakpointResolver.cpp
183–230

I just pulled those numbers out of thin air :-)
The first one is twice as big as the second one, because there is some filtering happening in between.

By name breakpoints don't use this function so a function regex wouldn't show the change. But the source regex does use this filter, so something like:

(lldb) break set -p ;

on a large source file might do it. That's a pretty silly thing to do, however. The only actual case where this might matter is if you have code that gets inlined all over the place and you set a breakpoint on the inlined code. But IRL I don't think the performance part of this change is really going to matter much.

lldb-bench now has a benchmark called br-by-regex that should profile this code. It should run tonight, so if merge this tomorrow or later we should be able to see the performance impact of this patch.

Except for the nit about the comment, this looks fine.

jingham accepted this revision.Aug 29 2018, 3:22 PM

Oh, yeah, gotta remember to check the box...

This revision is now accepted and ready to land.Aug 29 2018, 3:22 PM
This revision was automatically updated to reflect the committed changes.
aprantl marked an inline comment as done.Aug 29 2018, 4:33 PM

FYI: I tried to benchmark this using break set -A -p begin and similar things, but in all my experiments the variation between test runs was much larger than any difference with or without my patch. The filtering of the breakpoint locations is really cheap compared to everything else that is being done.

I'm not much surprised by that, but thanks for the experiments. As you say, that wasn't the primary purpose of this patch.