You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Joris Van Remoortere <jo...@gmail.com> on 2014/10/10 03:02:48 UTC
Review Request 26533: Memory cleanup: libprocess finalize
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26533/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Repository: mesos-git
Description
-------
Introduce a finalize() function to match the initialize() function in process.hpp. Use this at the unit test runner for libprocess to start getting more valuable valgrind feedback.
- Added cleanup of remaining Processes in ~ProcessManager() as a start.
Diffs
-----
3rdparty/libprocess/include/process/process.hpp 270ca28
3rdparty/libprocess/src/process.cpp d30ed63
3rdparty/libprocess/src/tests/main.cpp 0a3ba4e
Diff: https://reviews.apache.org/r/26533/diff/
Testing
-------
make check
support/mesos-style
valgrind 3rdparty/libprocess/tests (to see a reduction in definitely lost blocks)
Thanks,
Joris Van Remoortere
Re: Review Request 26533: Memory cleanup: libprocess finalize
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26533/
-----------------------------------------------------------
(Updated Oct. 10, 2014, 9:39 p.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Changes
-------
fix style issues.
Repository: mesos-git
Description
-------
Introduce a finalize() function to match the initialize() function in process.hpp. Use this at the unit test runner for libprocess to start getting more valuable valgrind feedback.
- Added cleanup of remaining Processes in ~ProcessManager() as a start.
Diffs (updated)
-----
3rdparty/libprocess/include/process/process.hpp 270ca28
3rdparty/libprocess/src/process.cpp d30ed63
3rdparty/libprocess/src/tests/main.cpp 0a3ba4e
Diff: https://reviews.apache.org/r/26533/diff/
Testing
-------
make check
support/mesos-style
valgrind 3rdparty/libprocess/tests (to see a reduction in definitely lost blocks)
Thanks,
Joris Van Remoortere
Re: Review Request 26533: Memory cleanup: libprocess finalize
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26533/#review56091
-----------------------------------------------------------
Patch looks great!
Reviews applied: [26533]
All tests passed.
- Mesos ReviewBot
On Oct. 10, 2014, 1:02 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26533/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 1:02 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduce a finalize() function to match the initialize() function in process.hpp. Use this at the unit test runner for libprocess to start getting more valuable valgrind feedback.
> - Added cleanup of remaining Processes in ~ProcessManager() as a start.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/process.hpp 270ca28
> 3rdparty/libprocess/src/process.cpp d30ed63
> 3rdparty/libprocess/src/tests/main.cpp 0a3ba4e
>
> Diff: https://reviews.apache.org/r/26533/diff/
>
>
> Testing
> -------
>
> make check
> support/mesos-style
> valgrind 3rdparty/libprocess/tests (to see a reduction in definitely lost blocks)
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 26533: Memory cleanup: libprocess finalize
Posted by Michael Park <mc...@gmail.com>.
> On Oct. 10, 2014, 6:02 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2449
> > <https://reviews.apache.org/r/26533/diff/1/?file=717191#file717191line2449>
> >
> > We usually prefer an explict representation:
> >
> > ```
> > if (ptr != nullptr) {
> > }
> > ```
> >
> > Also, probably we should create a ticket to convert all NULL to nullptr in our code base:)
Unfortunately, `nullptr` is not available in gcc-4.4 :(
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26533/#review56177
-----------------------------------------------------------
On Oct. 10, 2014, 1:02 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26533/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 1:02 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduce a finalize() function to match the initialize() function in process.hpp. Use this at the unit test runner for libprocess to start getting more valuable valgrind feedback.
> - Added cleanup of remaining Processes in ~ProcessManager() as a start.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/process.hpp 270ca28
> 3rdparty/libprocess/src/process.cpp d30ed63
> 3rdparty/libprocess/src/tests/main.cpp 0a3ba4e
>
> Diff: https://reviews.apache.org/r/26533/diff/
>
>
> Testing
> -------
>
> make check
> support/mesos-style
> valgrind 3rdparty/libprocess/tests (to see a reduction in definitely lost blocks)
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 26533: Memory cleanup: libprocess finalize
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26533/#review56177
-----------------------------------------------------------
Ship it!
LGTM.
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/26533/#comment96500>
We usually prefer an explict representation:
```
if (ptr != nullptr) {
}
```
Also, probably we should create a ticket to convert all NULL to nullptr in our code base:)
- Jie Yu
On Oct. 10, 2014, 1:02 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26533/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 1:02 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduce a finalize() function to match the initialize() function in process.hpp. Use this at the unit test runner for libprocess to start getting more valuable valgrind feedback.
> - Added cleanup of remaining Processes in ~ProcessManager() as a start.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/process.hpp 270ca28
> 3rdparty/libprocess/src/process.cpp d30ed63
> 3rdparty/libprocess/src/tests/main.cpp 0a3ba4e
>
> Diff: https://reviews.apache.org/r/26533/diff/
>
>
> Testing
> -------
>
> make check
> support/mesos-style
> valgrind 3rdparty/libprocess/tests (to see a reduction in definitely lost blocks)
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 26533: Memory cleanup: libprocess finalize
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26533/#review56186
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/26533/#comment96508>
We've used s/ptr/process throughout the rest of the code, please be consistent on variable naming!
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/26533/#comment96507>
Was I the only one that had to think about what you were doing here and why you had to do it the way you're doing it? ;-) Please add comments.
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/26533/#comment96509>
Why not use '!empty()'? Or if you use .size(), compare it with a number please.
3rdparty/libprocess/src/tests/main.cpp
<https://reviews.apache.org/r/26533/#comment96515>
We've used s/ret/result/ in the code base, let's stay consistent please.
- Benjamin Hindman
On Oct. 10, 2014, 1:02 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26533/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 1:02 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduce a finalize() function to match the initialize() function in process.hpp. Use this at the unit test runner for libprocess to start getting more valuable valgrind feedback.
> - Added cleanup of remaining Processes in ~ProcessManager() as a start.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/process.hpp 270ca28
> 3rdparty/libprocess/src/process.cpp d30ed63
> 3rdparty/libprocess/src/tests/main.cpp 0a3ba4e
>
> Diff: https://reviews.apache.org/r/26533/diff/
>
>
> Testing
> -------
>
> make check
> support/mesos-style
> valgrind 3rdparty/libprocess/tests (to see a reduction in definitely lost blocks)
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 26533: Memory cleanup: libprocess finalize
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26533/#review56172
-----------------------------------------------------------
Ship it!
Ship It!
- Dominic Hamon
On Oct. 9, 2014, 6:02 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26533/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2014, 6:02 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduce a finalize() function to match the initialize() function in process.hpp. Use this at the unit test runner for libprocess to start getting more valuable valgrind feedback.
> - Added cleanup of remaining Processes in ~ProcessManager() as a start.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/process.hpp 270ca28
> 3rdparty/libprocess/src/process.cpp d30ed63
> 3rdparty/libprocess/src/tests/main.cpp 0a3ba4e
>
> Diff: https://reviews.apache.org/r/26533/diff/
>
>
> Testing
> -------
>
> make check
> support/mesos-style
> valgrind 3rdparty/libprocess/tests (to see a reduction in definitely lost blocks)
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 26533: Memory cleanup: libprocess finalize
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26533/#review56192
-----------------------------------------------------------
3rdparty/libprocess/include/process/process.hpp
<https://reviews.apache.org/r/26533/#comment96518>
Since no one caught this: s/c/C/
- Ben Mahler
On Oct. 10, 2014, 1:02 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26533/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 1:02 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduce a finalize() function to match the initialize() function in process.hpp. Use this at the unit test runner for libprocess to start getting more valuable valgrind feedback.
> - Added cleanup of remaining Processes in ~ProcessManager() as a start.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/process.hpp 270ca28
> 3rdparty/libprocess/src/process.cpp d30ed63
> 3rdparty/libprocess/src/tests/main.cpp 0a3ba4e
>
> Diff: https://reviews.apache.org/r/26533/diff/
>
>
> Testing
> -------
>
> make check
> support/mesos-style
> valgrind 3rdparty/libprocess/tests (to see a reduction in definitely lost blocks)
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 26533: Memory cleanup: libprocess finalize
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26533/#review56173
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/26533/#comment96494>
Bring { down on newline
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/26533/#comment96495>
Same here
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/26533/#comment96493>
I know that nullptr is the advised way from the Google Style Guide - but we use the C-style NULL. Let's use that here and do a scan for all cases (when/if we agree on the move)
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/26533/#comment96497>
Same here
- Niklas Nielsen
On Oct. 9, 2014, 6:02 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26533/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2014, 6:02 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduce a finalize() function to match the initialize() function in process.hpp. Use this at the unit test runner for libprocess to start getting more valuable valgrind feedback.
> - Added cleanup of remaining Processes in ~ProcessManager() as a start.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/process.hpp 270ca28
> 3rdparty/libprocess/src/process.cpp d30ed63
> 3rdparty/libprocess/src/tests/main.cpp 0a3ba4e
>
> Diff: https://reviews.apache.org/r/26533/diff/
>
>
> Testing
> -------
>
> make check
> support/mesos-style
> valgrind 3rdparty/libprocess/tests (to see a reduction in definitely lost blocks)
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 26533: Memory cleanup: libprocess finalize
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Oct. 10, 2014, 5:04 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1453
> > <https://reviews.apache.org/r/26533/diff/1/?file=717191#file717191line1453>
> >
> > should these be std::unique_ptr instead? (i'm currently adding support for this to the libprocess configure check).
Indeed. I have some follow up patches that will try to tackle the actual clean shutdown. This initial patch it to try and introduce a place to close the loop.
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26533/#review56166
-----------------------------------------------------------
On Oct. 10, 2014, 1:02 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26533/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 1:02 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduce a finalize() function to match the initialize() function in process.hpp. Use this at the unit test runner for libprocess to start getting more valuable valgrind feedback.
> - Added cleanup of remaining Processes in ~ProcessManager() as a start.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/process.hpp 270ca28
> 3rdparty/libprocess/src/process.cpp d30ed63
> 3rdparty/libprocess/src/tests/main.cpp 0a3ba4e
>
> Diff: https://reviews.apache.org/r/26533/diff/
>
>
> Testing
> -------
>
> make check
> support/mesos-style
> valgrind 3rdparty/libprocess/tests (to see a reduction in definitely lost blocks)
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 26533: Memory cleanup: libprocess finalize
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26533/#review56166
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/26533/#comment96486>
should these be std::unique_ptr instead? (i'm currently adding support for this to the libprocess configure check).
- Dominic Hamon
On Oct. 9, 2014, 6:02 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26533/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2014, 6:02 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduce a finalize() function to match the initialize() function in process.hpp. Use this at the unit test runner for libprocess to start getting more valuable valgrind feedback.
> - Added cleanup of remaining Processes in ~ProcessManager() as a start.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/process.hpp 270ca28
> 3rdparty/libprocess/src/process.cpp d30ed63
> 3rdparty/libprocess/src/tests/main.cpp 0a3ba4e
>
> Diff: https://reviews.apache.org/r/26533/diff/
>
>
> Testing
> -------
>
> make check
> support/mesos-style
> valgrind 3rdparty/libprocess/tests (to see a reduction in definitely lost blocks)
>
>
> Thanks,
>
> Joris Van Remoortere
>
>