mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 63367: Added overloads for strings::startsWith and strings::endsWith().
Date Wed, 24 Jan 2018 17:02:12 GMT

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



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));
}
```

- Michael Park


On Jan. 24, 2018, 7:12 a.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63367/
> -----------------------------------------------------------
> 
> (Updated Jan. 24, 2018, 7:12 a.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
> 
>


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