> On Jan. 24, 2018, 5:02 p.m., Michael Park wrote:
> > I would prefer a more general, systematic approach to this situation.
> > I really don't want to be deal with adding `string` and `const char []`
> > overloads everywhere. Ideally, we'd have a `string_view` class that we
> > could use instead of `string` here.
> >
> > Now, as far as performance goes, there are a couple of things to note.
> > (1) It seems like the claim is that it speeds up the tests? by how much?
> > (2) The claim that we avoid an unnecessary allocation isn't actually
> > true considering that the strings (at least in our tests) probably
> > fit within the short-string optimization.
> > (3) Is this change by itself a big win somewhere? or will it require
> > a systematic change where `string` can be a `string_view`?
> > If the former, I'd be fine with optimizing these functions, but
> > if it's the former, I'd say we should invest in a `string_view` class.
> >
> > I quite like performance, but some more evidence would be helpful here.
> >
> > Now, under the assumption that the performance argument to be justified,
> > we can simply make the existing function a template and generalize the
> > logic:
> >
> > ```cpp
> > template <typename Stringish>
> > bool startsWith(const string& s, const Stringish& prefix) {
> > return s.size() >= distance(begin(prefix), end(prefix)) &&
> > equal(begin(prefix), end(prefix), begin(s));
> > }
> > ```
I don't think the template version will work, unless there's an overload of
`std::end` for `const char*`? In general, the standard library also includes
overloads for `const char*` for most functions taking string arguments
(including the proposed `std::string_view::starts_with()`) so I doubt there's
some magic bullet to avoid that.
If we plan to embrace `string_view` in the future, we can still upgrade the
signature from `const char*` to `string_view` once we made the switch to
C++17; nothing in this review makes it harder to introduce that. (unless you
want to avoid ABI breakage, but this would also prevent the template solution
above)
For the performance, I actually didn't do any measurements, but it seems
reasonable to assume that doing something will take more time than doing
nothing. Even if the string will be SSO-able on many platforms (not on all
though, we still build for e.g. Ubuntu 14.04 where the C++11 ABI isn't used by
default), it still has to be constructed first, which isn't free:
https://godbolt.org/g/SUJ9Tn
When testing this function in isolation, I can see about 30% speedup. In the
context of mesos, I don't expect to see a huge impact, but even a free 0.1%
would be nice to have, in particular in a function that was subject to
performance concerns in the past. (MESOS-5715)
Finally, having written all of this, my main motivation for this patch was
actually purely aesthetic: Paying a non-zero overhead to use an abstraction
just doesn't feel right in C++, no matter how small the practical implications.
;)
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63367/#review196140
-----------------------------------------------------------
On Jan. 24, 2018, 3:12 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63367/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2018, 3:12 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This saves an unnecessary memory allocation when
> testing if a string starts or ends with a string literal,
> which accounts for almost all usages of these functions
> in Mesos and in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/strings.hpp
> 067a7923c02342bccd9be1136a981fd6b0e0e9b4
> 3rdparty/stout/tests/strings_tests.cpp
> 395540aad88c76a66a43a54edfe9ca1a2d46d3b4
>
>
> Diff: https://reviews.apache.org/r/63367/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
