mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 63367: Added overloads for strings::startsWith and strings::endsWith().
Date Wed, 11 Apr 2018 02:01:58 GMT


> 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));
> > }
> > ```
> 
> Benno Evers wrote:
>     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. ;)

Since C++20 will include starts_with and ends_with, it seems ok to just mimic the C++20 signatures
now and put a TODO to remove these when we migrate to C++20.


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63367/#review196140
-----------------------------------------------------------


On April 3, 2018, 4:27 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63367/
> -----------------------------------------------------------
> 
> (Updated April 3, 2018, 4:27 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/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message