You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2019/02/25 21:20:05 UTC
Review Request 70053: Used std::string as buffer instead of manually
managed dynamic array.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70053/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Repository: mesos
Description
-------
This is a non-functional change to avoid manual memory management. By
using a `std::string` instead of an explicitly heap-allocated buffer the
used pattern might even avoid hitting the heap altogether due to the SSO
but this function might be runtime bounded by I/O anyway.
Diffs
-----
3rdparty/stout/include/stout/os/posix/mktemp.hpp 8dab2599f13c3e1dab109423c8a938ec16540aaf
Diff: https://reviews.apache.org/r/70053/diff/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 70053: Used std::string as buffer instead of
manually managed dynamic array.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70053/#review213196
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70046, 70053]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 25, 2019, 1:20 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70053/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2019, 1:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a non-functional change to avoid manual memory management. By
> using a `std::string` instead of an explicitly heap-allocated buffer the
> used pattern might even avoid hitting the heap altogether due to the SSO
> but this function might be runtime bounded by I/O anyway.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/posix/mktemp.hpp 8dab2599f13c3e1dab109423c8a938ec16540aaf
>
>
> Diff: https://reviews.apache.org/r/70053/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70053: Used std::string as buffer instead of
manually managed dynamic array.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70053/#review213192
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70046, 70053]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 25, 2019, 9:20 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70053/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2019, 9:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a non-functional change to avoid manual memory management. By
> using a `std::string` instead of an explicitly heap-allocated buffer the
> used pattern might even avoid hitting the heap altogether due to the SSO
> but this function might be runtime bounded by I/O anyway.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/posix/mktemp.hpp 8dab2599f13c3e1dab109423c8a938ec16540aaf
>
>
> Diff: https://reviews.apache.org/r/70053/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70053: Used std::string as buffer instead of
manually managed dynamic array.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70053/#review213679
-----------------------------------------------------------
PASS: Mesos patch 70053 was successfully built and tested.
Reviews applied: `['70053']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2931/mesos-review-70053
- Mesos Reviewbot Windows
On Feb. 25, 2019, 9:20 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70053/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2019, 9:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a non-functional change to avoid manual memory management. By
> using a `std::string` instead of an explicitly heap-allocated buffer the
> used pattern might even avoid hitting the heap altogether due to the SSO
> but this function might be runtime bounded by I/O anyway.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/posix/mktemp.hpp 8dab2599f13c3e1dab109423c8a938ec16540aaf
>
>
> Diff: https://reviews.apache.org/r/70053/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70053: Used std::string as buffer instead of
manually managed dynamic array.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 25, 2019, 11:05 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/os/posix/mktemp.hpp
> > Lines 39-57 (original), 39-57 (patched)
> > <https://reviews.apache.org/r/70053/diff/1/?file=2126763#file2126763line39>
> >
> > From what I understand, C++11 already guarantees for us that `_path[_path.size()] == '\0'` and so we shouldn't need this +1 and '\0' logic?
> >
> > ```
> > std::string _path(path);
> >
> > int_fd fd = ::mkstemp(&_path[0]);
> >
> > ...
> >
> > return _path;
> > ```
I read that, too.
Note that here we do not evaluate `_path[_path.size()]` directly, but instead pass `&_path[0]` to a function expecting a null-terminated string. AFAICT no C++ standard makes promises about `string` actually null-terminating strings internally (e.g., `_path.data()` which would be equivalent to `&_path[0]` isn't per se uaranteed to point to a null-terminated string in even the most recent standard), and the only reference to automatically null-terminating strings I could find were related to explicitly accessing the `size()`'th element, and `string::c_str`, neither of which return the internal buffer we are using here.
I believe what I do here is needed.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70053/#review213187
-----------------------------------------------------------
On Feb. 25, 2019, 10:20 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70053/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2019, 10:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a non-functional change to avoid manual memory management. By
> using a `std::string` instead of an explicitly heap-allocated buffer the
> used pattern might even avoid hitting the heap altogether due to the SSO
> but this function might be runtime bounded by I/O anyway.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/posix/mktemp.hpp 8dab2599f13c3e1dab109423c8a938ec16540aaf
>
>
> Diff: https://reviews.apache.org/r/70053/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70053: Used std::string as buffer instead of
manually managed dynamic array.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Feb. 25, 2019, 10:05 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/os/posix/mktemp.hpp
> > Lines 39-57 (original), 39-57 (patched)
> > <https://reviews.apache.org/r/70053/diff/1/?file=2126763#file2126763line39>
> >
> > From what I understand, C++11 already guarantees for us that `_path[_path.size()] == '\0'` and so we shouldn't need this +1 and '\0' logic?
> >
> > ```
> > std::string _path(path);
> >
> > int_fd fd = ::mkstemp(&_path[0]);
> >
> > ...
> >
> > return _path;
> > ```
>
> Benjamin Bannier wrote:
> I read that, too.
>
> Note that here we do not evaluate `_path[_path.size()]` directly, but instead pass `&_path[0]` to a function expecting a null-terminated string. AFAICT no C++ standard makes promises about `string` actually null-terminating strings internally (e.g., `_path.data()` which would be equivalent to `&_path[0]` isn't per se uaranteed to point to a null-terminated string in even the most recent standard), and the only reference to automatically null-terminating strings I could find were related to explicitly accessing the `size()`'th element, and `string::c_str`, neither of which return the internal buffer we are using here.
>
> I believe what I do here is needed.
I was chatting with mpark, it seems the following in the standard enforces it:
> data() / c_str():
>
> A pointer p such that p + i == &operator[](i) for each i in [0, size()]
in combination with:
> const_reference operator[](size_type pos) const
>
> Returns: *(begin() + pos) if pos < size(). Otherwise, returns a reference to an object of type
charT with value charT(), where modifying the object to any value other than charT() leads to
undefined behavior
i.e. if you pass s.data(), it needs to be null terminated. Not sure where you saw the information about .data()?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70053/#review213187
-----------------------------------------------------------
On Feb. 25, 2019, 9:20 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70053/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2019, 9:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a non-functional change to avoid manual memory management. By
> using a `std::string` instead of an explicitly heap-allocated buffer the
> used pattern might even avoid hitting the heap altogether due to the SSO
> but this function might be runtime bounded by I/O anyway.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/posix/mktemp.hpp 8dab2599f13c3e1dab109423c8a938ec16540aaf
>
>
> Diff: https://reviews.apache.org/r/70053/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70053: Used std::string as buffer instead of
manually managed dynamic array.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70053/#review213187
-----------------------------------------------------------
3rdparty/stout/include/stout/os/posix/mktemp.hpp
Lines 39-57 (original), 39-57 (patched)
<https://reviews.apache.org/r/70053/#comment299051>
From what I understand, C++11 already guarantees for us that `_path[_path.size()] == '\0'` and so we shouldn't need this +1 and '\0' logic?
```
std::string _path(path);
int_fd fd = ::mkstemp(&_path[0]);
...
return _path;
```
- Benjamin Mahler
On Feb. 25, 2019, 9:20 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70053/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2019, 9:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a non-functional change to avoid manual memory management. By
> using a `std::string` instead of an explicitly heap-allocated buffer the
> used pattern might even avoid hitting the heap altogether due to the SSO
> but this function might be runtime bounded by I/O anyway.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/posix/mktemp.hpp 8dab2599f13c3e1dab109423c8a938ec16540aaf
>
>
> Diff: https://reviews.apache.org/r/70053/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>