Re: Review Request 26533: Memory cleanup: libprocess finalize
--- 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
Re: Review Request 26533: Memory cleanup: libprocess finalize
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
---
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
--- 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
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
--- 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
--- 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
