[Lldb-commits] [lldb] r281273 - This is the main part of a change to add breakpoint save and restore to lldb.

[Zachary Turner via lldb-commits]

On Mon, Sep 12, 2016 at 6:24 PM Jim Ingham <jingham at apple.com> wrote:

> I see the whole content, but I'll reply to this one so the reply doesn't
> get truncated on your end...
>
> > On Sep 12, 2016, at 6:03 PM, Zachary Turner <zturner at google.com> wrote:
> >
> >
> > Immediately, very little.  A small amount of performance, since
> comparing StringRefs is faster than comparing null terminated stings, since
> the length is stored with the string it can use memcmp instead of strcmp.
> >
> > From a big picture perspective, quite a lot IMO.  StringRef has numerous
> methods which are extremely useful and not easy to reproduce correctly.
> Also, If you do the comparison many times, you only pay for the length
> computation once.
> >
> > Also, this email is so long that it's truncating in my email reader.  So
> I'm not sure if the second part of my email went through.  It's possible
> your reader is better than mine at handling large emails, but I'm copying
> the answer to your second question below just in case you also didn't see
> it.
> >
> > Thanks for the comments.
> > >
> > >   I don't see the benefit of using StringRef's to return all the key
> names.  I'm generally only ever going to pass them to the StructuredData
> API's, which makes them into StringRef's transparently on the way in.  How
> would building StringRefs help here?
> >
> > >   You also suggested changing a bunch of BreakpointOption API to
> return StringRef's.  OTOH this CL just mechanically changed from m_options
> to m_options_up, so changing the API was not part of the patches intent.
> OTOH most of these options (condition, thread name, etc) can take and
> return NULL strings.  I didn't think StringRef's handled null strings well,
> was I wrong about that?  And again, what would I gain by making these
> StringRef's?  I'm just going to stash them away in some storage the object
> owns, and I'm not going to ever share the storage with the caller.  So
> least common denominator (const char *) seems a better choice here.  If the
> caller wants a StringRef they are welcome to make one.
> >
> > Right, but making the StringRef incurs a length computation.  That's not
> something you want to do over and over.  It's guaranteed someone somewhere
> is going to compute the length, so it's better to do it once upfront, and
> allow everyone else to never have to do it again.
>
> Maybe I don't understand how StringRef's work.  I thought they just
> wrapped some foreign storage - a string constant, char * or std::string?
> So for the length computation to be shared for an object handing out
> StringRef's, the object would have to keep both the string and it's
> associated StringRef.  If the functions just RETURN a StringRef that wraps
> a string constant, you'll calculate the length every time.  So IIUC:
>
> static const char *GetSerializationKey() { return "Breakpoint"; }
>
> becomes:
>
> static StringRef GetSerializationKey() { static const StringRef
> contents("Breakpoint"); return contents; }
>
> except now this has a non-trivial constructor, so I should really put a
> std::once around the initializer, right?  That just seems like way more
> trouble than it is worth to keep from computing a length a couple of times.
>
> >
> > On the other hand, using a StringRef gives you many advantages.  Unless
> you know every possible way in which these strings will ever be used, who
> knows what someone might want to do with it?  What if someone wants to take
> one of these strings, check if some other string starts with it, and chop
> it off if so?  You could either write:
> >
> > if (strncmp(GetName(), str.c_str(), strlen(GetName()) == 0)
> >     str2 = str.substr(strlen(GetName()));
> >
> > which there's a good chance will be either wrong or suboptimal, or you
> could write:
> >
> > str.consume_front(GetName());
> >
> > which is both easier to understand, obviously correct, and optimal from
> a performance standpoint.
> >
> > const char* should almost never be used for anything unless you
> absolutely must interface with a C-api for which there is no good
> equivalent on StringRef (very rare).
>
> Hum.  I would say "If you want to start parsing up a string, put it in a
> StringRef, you'll like it..."  But if you are handing out a string
> constant, "const char *" is fine, and the consumers can dress it up if they
> want.
>
> >
> > Since we currently use const char* in many places, this sometimes makies
> interfacing difficult, but as more common classes move to StringRef, this
> will go away and almost all code will become faster and easier to
> understand.
> >
> > You are right that StringRefs don't handle null strings, but they do
> handle empty strings, and it's not common that empty and null need to be
> treated as distinct values.
>
> The classes you reference treat nullptr as "not set".  I could go change
> that too, but I'd rather write some tests.
>
> Jim
>
>
>
Also thought of something else.  Many compilers optimize strlen of string
literals so it computes it at compile-time.  (Some don't, and for those I'm
in the process of adding a new new constructor to StringRef that achieves
the same effect).

So, when you construct a StringRef from a string literal, there are
actually *0* runtime length computations.  Now sure, a function like

const char * getFizzbuzz() { return "Fizzbuzz"; }

is going to be inlined, so if you write

int len = strlen(getFizzbuzz())

on these compilers you will not incur a length computation.  But if you do
this:

int work(const char *str) {
  int x = ::strlen(str);
  // stuff
}

work(getFizzbuzz());

then there's a good chance that `work` isn't inlined, and this won't be
optimized (plus, it will never be optimized on compilers which don't
support this optimization).  On the other hand, if you have the following:

StringRef getFizzbuzz() { return "Fizzbuzz"; }

int work(StringRef str) {
  int x = str.size();
}

work(getFizzbuzz());

Then there will be no length computations on any compiler regardless of
optimization settings, even in debug mode.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160913/365c64db/attachment.html>