[Lldb-commits] [lldb] r282079 - Make lldb::Regex use StringRef.

[Zachary Turner via lldb-commits]

But the thing is, there were plenty of copies being made before.  People
just didn't see them.  StringRef provides functions like find_first,
substr, and all kinds of stuff.  So before, any time someone wanted to use
one of those functions, what did they do?  Put it in a std::string!
There's your copy.  Even worse, you might think that once you've got a
std::string, at least you don't have to make any more copies, but even
*that* isn't true.  If you want to call std::string::substr(), that
mandates a copy.  Even if you don't put it in a std::string, if you use
strchr or whatever to find the index and calculate your substring c-style,
and you want to pass that to a function taking a const char*, now you have
to put it in a std::string so you can null terminate it, so there's another
copy.

So yes, there's a few more temporary ones while the conversion is in
progress, but there's also some that are removed such as in the above
examples.  I'm not worried too much about the temporary ones because in the
long run the net effect will be a huge reduction in number of copies and
strlens.

On Wed, Sep 21, 2016 at 5:23 PM Greg Clayton <gclayton at apple.com> wrote:

>
> > On Sep 21, 2016, at 5:14 PM, Zachary Turner <zturner at google.com> wrote:
> >
> >
> >
> > On Wed, Sep 21, 2016 at 5:00 PM Greg Clayton <gclayton at apple.com> wrote:
> > Please submit a change requests when doing these kinds of things and let
> people comment on the changes before committing such things.
> >
> > We deleted functions that were correctly using "const char *" like:
> >
> >   bool Execute(llvm::StringRef string, Match *match = nullptr) const;
> >   bool Execute(const char *, Match * = nullptr) = delete;
> >
> > Yet inside these functions you do "string.str().c_str()" because these
> need to be null terminated? this indicates that StringRef is NOT the
> correct choice here. You will make heap copy of all strings (if they are
> long enough and regex matching string can be very long) that you compile or
> execute. We already make a copy of the string when we compile, so StringRef
> didn't really save us anything on compile and execute is always making a
> copy just to run the expression on. I am fine with both being there in case
> one might be more efficient, but taking them out just to use a less
> efficient version that uses llvm::StringRef is not the kind of changes we
> should be doing all over.
> >
> > We will make copies of all strings in all of the following changes:
> >
> > - Unneeded copy:
> >
> > -        new TypeNameSpecifierImpl(regex->GetText(), true));
> > +        new TypeNameSpecifierImpl(regex->GetText().str().c_str(),
> true));
> > All of these copies are only temporary.  As I've said over and over,
> once everything is StringRef all the way down, the copies all disappear.
> It's only a copy because TypeNameSpecifierImpl doesn't take a STringRef,
> and I didn't want to change the whole codebase all at once, but rather
> incrementally.
>
> As part of these changes we absolutely should add StringRef to these
> classes and we should just make it part of the changes. It know these
> strings are temporary, I am not worried about memory, I am worried about
> efficiency of the heap copy of the string that temporarily is created
> during this call. If things were taking "const char *" before, copies were
> not being made. If they were being made, then they were probably being made
> for good reason. I would love to see the places where all of these copies
> were removed, but if they were "const char *" there were no copies going
> in, unless they needed to. If they didn't need to make the copies, then we
> can fix that function.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160922/546cc948/attachment.html>