You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2015/11/04 19:58:58 UTC
Review Request 39949: Document and simplify libprocess initialization
synchronization logic.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
Repository: mesos
Description
-------
The initialization synchronization logic contains three conditions, which check:
1) Was `initialize` called and is it done?
2) Was `initialize` called and is it not done?
3) Are you the first to call `initialize`?
Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
Diffs
-----
3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
Diff: https://reviews.apache.org/r/39949/diff/
Testing
-------
`make check`
Thanks,
Joseph Wu
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/#review105128
-----------------------------------------------------------
Ship it!
Ship It!
- Neil Conway
On Nov. 4, 2015, 8:24 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 8:24 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Renamed `initialized` to `initialize_started`.
> * Renamed `initializing` to `initialize_complete`.
> * Removed the (2) condition, described below:
>
> The initialization synchronization logic contains three conditions, which check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
>
> Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
>
> The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/39949/diff/
>
>
> Testing
> -------
>
> `make check`
>
> Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` with:
> ```
>
> const size_t numThreads = 50;
>
> std::thread* runningThreads[numThreads];
>
> // Create additional threads.
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i] = new std::thread([]() {
> process::initialize();
> });
> }
>
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i]->join();
> delete runningThreads[i];
> }
> ```
> (Also added `#include <thread>` to the header).
>
> Rebuilt `libprocess-tests` with the modification and ran it a few times.
> `3rdparty/libprocess/libprocess-tests`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/#review105564
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39948, 39949]
All tests passed.
- Mesos ReviewBot
On Nov. 6, 2015, 10:11 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2015, 10:11 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3820
> https://issues.apache.org/jira/browse/MESOS-3820
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Renamed `initialized` to `initialize_started`.
> * Renamed `initializing` to `initialize_complete`.
> * Removed the (2) condition, described below:
>
> The initialization synchronization logic contains three conditions, which check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
>
> Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
>
> The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/39949/diff/
>
>
> Testing
> -------
>
> `make check`
>
> Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` with:
> ```
>
> const size_t numThreads = 50;
>
> std::thread* runningThreads[numThreads];
>
> // Create additional threads.
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i] = new std::thread([]() {
> process::initialize();
> });
> }
>
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i]->join();
> delete runningThreads[i];
> }
> ```
> (Also added `#include <thread>` to the header).
>
> Rebuilt `libprocess-tests` with the modification and ran it a few times.
> `3rdparty/libprocess/libprocess-tests`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Nov. 6, 2015, 5:50 p.m., Mesos ReviewBot wrote:
> > Bad patch!
> >
> > Reviews applied: [39948]
> >
> > Failed command: ./support/apply-review.sh -n -r 39948
> >
> > Error:
> > 2015-11-07 01:50:12 URL:https://reviews.apache.org/r/39948/diff/raw/ [2087/2087] -> "39948.patch" [1]
> > error: patch failed: 3rdparty/libprocess/src/process.cpp:739
> > error: 3rdparty/libprocess/src/process.cpp: patch does not apply
> > Failed to apply patch
Hmm... That's weird. I can apply just fine on the head commit (at the time, c68963297c9bec52cd6efb269454340b824ae931).
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/#review105561
-----------------------------------------------------------
On Nov. 6, 2015, 2:11 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2015, 2:11 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3820
> https://issues.apache.org/jira/browse/MESOS-3820
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Renamed `initialized` to `initialize_started`.
> * Renamed `initializing` to `initialize_complete`.
> * Removed the (2) condition, described below:
>
> The initialization synchronization logic contains three conditions, which check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
>
> Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
>
> The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/39949/diff/
>
>
> Testing
> -------
>
> `make check`
>
> Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` with:
> ```
>
> const size_t numThreads = 50;
>
> std::thread* runningThreads[numThreads];
>
> // Create additional threads.
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i] = new std::thread([]() {
> process::initialize();
> });
> }
>
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i]->join();
> delete runningThreads[i];
> }
> ```
> (Also added `#include <thread>` to the header).
>
> Rebuilt `libprocess-tests` with the modification and ran it a few times.
> `3rdparty/libprocess/libprocess-tests`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/#review105561
-----------------------------------------------------------
Bad patch!
Reviews applied: [39948]
Failed command: ./support/apply-review.sh -n -r 39948
Error:
2015-11-07 01:50:12 URL:https://reviews.apache.org/r/39948/diff/raw/ [2087/2087] -> "39948.patch" [1]
error: patch failed: 3rdparty/libprocess/src/process.cpp:739
error: 3rdparty/libprocess/src/process.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On Nov. 6, 2015, 10:11 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2015, 10:11 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3820
> https://issues.apache.org/jira/browse/MESOS-3820
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Renamed `initialized` to `initialize_started`.
> * Renamed `initializing` to `initialize_complete`.
> * Removed the (2) condition, described below:
>
> The initialization synchronization logic contains three conditions, which check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
>
> Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
>
> The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/39949/diff/
>
>
> Testing
> -------
>
> `make check`
>
> Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` with:
> ```
>
> const size_t numThreads = 50;
>
> std::thread* runningThreads[numThreads];
>
> // Create additional threads.
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i] = new std::thread([]() {
> process::initialize();
> });
> }
>
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i]->join();
> delete runningThreads[i];
> }
> ```
> (Also added `#include <thread>` to the header).
>
> Rebuilt `libprocess-tests` with the modification and ran it a few times.
> `3rdparty/libprocess/libprocess-tests`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/#review107634
-----------------------------------------------------------
Ship it!
I'll add a comment as to why we use atomic before committing.
- Joris Van Remoortere
On Nov. 20, 2015, 7:16 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2015, 7:16 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3864
> https://issues.apache.org/jira/browse/MESOS-3864
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Renamed `initialized` to `initialize_started`.
> * Renamed `initializing` to `initialize_complete`.
> * Removed the (2) condition, described below:
>
> The initialization synchronization logic contains three conditions, which check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
>
> Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
>
> The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 8fa0594b671969c2713d1b361f2dbbb07d25b3a8
>
> Diff: https://reviews.apache.org/r/39949/diff/
>
>
> Testing
> -------
>
> `make check`
>
> Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` with:
> ```
>
> const size_t numThreads = 50;
>
> std::thread* runningThreads[numThreads];
>
> // Create additional threads.
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i] = new std::thread([]() {
> process::initialize();
> });
> }
>
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i]->join();
> delete runningThreads[i];
> }
> ```
> (Also added `#include <thread>` to the header).
>
> Rebuilt `libprocess-tests` with the modification and ran it a few times.
> `3rdparty/libprocess/libprocess-tests`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/
-----------------------------------------------------------
(Updated Nov. 20, 2015, 11:16 a.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil Conway.
Changes
-------
Rebase.
Bugs: MESOS-3864
https://issues.apache.org/jira/browse/MESOS-3864
Repository: mesos
Description
-------
* Renamed `initialized` to `initialize_started`.
* Renamed `initializing` to `initialize_complete`.
* Removed the (2) condition, described below:
The initialization synchronization logic contains three conditions, which check:
1) Was `initialize` called and is it done?
2) Was `initialize` called and is it not done?
3) Are you the first to call `initialize`?
Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
Diffs (updated)
-----
3rdparty/libprocess/src/process.cpp 8fa0594b671969c2713d1b361f2dbbb07d25b3a8
Diff: https://reviews.apache.org/r/39949/diff/
Testing
-------
`make check`
Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` with:
```
const size_t numThreads = 50;
std::thread* runningThreads[numThreads];
// Create additional threads.
for (size_t i = 0; i < numThreads; i++) {
runningThreads[i] = new std::thread([]() {
process::initialize();
});
}
for (size_t i = 0; i < numThreads; i++) {
runningThreads[i]->join();
delete runningThreads[i];
}
```
(Also added `#include <thread>` to the header).
Rebuilt `libprocess-tests` with the modification and ran it a few times.
`3rdparty/libprocess/libprocess-tests`
Thanks,
Joseph Wu
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/
-----------------------------------------------------------
(Updated Nov. 9, 2015, 2:23 p.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil Conway.
Bugs: MESOS-3864
https://issues.apache.org/jira/browse/MESOS-3864
Repository: mesos
Description
-------
* Renamed `initialized` to `initialize_started`.
* Renamed `initializing` to `initialize_complete`.
* Removed the (2) condition, described below:
The initialization synchronization logic contains three conditions, which check:
1) Was `initialize` called and is it done?
2) Was `initialize` called and is it not done?
3) Are you the first to call `initialize`?
Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
Diffs
-----
3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
Diff: https://reviews.apache.org/r/39949/diff/
Testing
-------
`make check`
Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` with:
```
const size_t numThreads = 50;
std::thread* runningThreads[numThreads];
// Create additional threads.
for (size_t i = 0; i < numThreads; i++) {
runningThreads[i] = new std::thread([]() {
process::initialize();
});
}
for (size_t i = 0; i < numThreads; i++) {
runningThreads[i]->join();
delete runningThreads[i];
}
```
(Also added `#include <thread>` to the header).
Rebuilt `libprocess-tests` with the modification and ran it a few times.
`3rdparty/libprocess/libprocess-tests`
Thanks,
Joseph Wu
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Nov. 7, 2015, 7:35 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 742-770
> > <https://reviews.apache.org/r/39949/diff/2/?file=1115936#file1115936line742>
> >
> > Could we document why we're not just using process::Once to clean this up?
> >
> > The only thing that comes to mind is the performance implications of changing from a spin loop to the mutex condition variable approach, given that this gets called all over the place.
I don't know why we don't use `Once` either. Perhaps because the synchronization logic predates `Once`? (And that wouldn't be a very good reason.)
`Once` was added April 30, 2012:
https://github.com/apache/mesos/commit/6c3b107e4e02d5ba0673eb3145d71ec9d256a639#diff-0eebc8689450916990abe080d86c2acb
The libprocess synchronization logic has been mostly unchanged since June 04, 2011:
https://github.com/apache/mesos/commit/cd757cf75637c92c438bf4cd22f21ba1b5be702f#diff-128d3b56fc8c9ec0176fdbadcfd11fc2
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/#review105602
-----------------------------------------------------------
On Nov. 6, 2015, 2:11 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2015, 2:11 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3820
> https://issues.apache.org/jira/browse/MESOS-3820
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Renamed `initialized` to `initialize_started`.
> * Renamed `initializing` to `initialize_complete`.
> * Removed the (2) condition, described below:
>
> The initialization synchronization logic contains three conditions, which check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
>
> Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
>
> The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/39949/diff/
>
>
> Testing
> -------
>
> `make check`
>
> Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` with:
> ```
>
> const size_t numThreads = 50;
>
> std::thread* runningThreads[numThreads];
>
> // Create additional threads.
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i] = new std::thread([]() {
> process::initialize();
> });
> }
>
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i]->join();
> delete runningThreads[i];
> }
> ```
> (Also added `#include <thread>` to the header).
>
> Rebuilt `libprocess-tests` with the modification and ran it a few times.
> `3rdparty/libprocess/libprocess-tests`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/#review105602
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp (lines 742 - 770)
<https://reviews.apache.org/r/39949/#comment164228>
Could we document why we're not just using process::Once to clean this up?
The only thing that comes to mind is the performance implications of changing from a spin loop to the mutex condition variable approach, given that this gets called all over the place.
- Ben Mahler
On Nov. 6, 2015, 10:11 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2015, 10:11 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3820
> https://issues.apache.org/jira/browse/MESOS-3820
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Renamed `initialized` to `initialize_started`.
> * Renamed `initializing` to `initialize_complete`.
> * Removed the (2) condition, described below:
>
> The initialization synchronization logic contains three conditions, which check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
>
> Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
>
> The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/39949/diff/
>
>
> Testing
> -------
>
> `make check`
>
> Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` with:
> ```
>
> const size_t numThreads = 50;
>
> std::thread* runningThreads[numThreads];
>
> // Create additional threads.
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i] = new std::thread([]() {
> process::initialize();
> });
> }
>
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i]->join();
> delete runningThreads[i];
> }
> ```
> (Also added `#include <thread>` to the header).
>
> Rebuilt `libprocess-tests` with the modification and ran it a few times.
> `3rdparty/libprocess/libprocess-tests`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/
-----------------------------------------------------------
(Updated Nov. 6, 2015, 2:11 p.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil Conway.
Bugs: MESOS-3820
https://issues.apache.org/jira/browse/MESOS-3820
Repository: mesos
Description
-------
* Renamed `initialized` to `initialize_started`.
* Renamed `initializing` to `initialize_complete`.
* Removed the (2) condition, described below:
The initialization synchronization logic contains three conditions, which check:
1) Was `initialize` called and is it done?
2) Was `initialize` called and is it not done?
3) Are you the first to call `initialize`?
Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
Diffs
-----
3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
Diff: https://reviews.apache.org/r/39949/diff/
Testing
-------
`make check`
Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` with:
```
const size_t numThreads = 50;
std::thread* runningThreads[numThreads];
// Create additional threads.
for (size_t i = 0; i < numThreads; i++) {
runningThreads[i] = new std::thread([]() {
process::initialize();
});
}
for (size_t i = 0; i < numThreads; i++) {
runningThreads[i]->join();
delete runningThreads[i];
}
```
(Also added `#include <thread>` to the header).
Rebuilt `libprocess-tests` with the modification and ran it a few times.
`3rdparty/libprocess/libprocess-tests`
Thanks,
Joseph Wu
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/
-----------------------------------------------------------
(Updated Nov. 4, 2015, 12:24 p.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil Conway.
Changes
-------
Renamed variables to more closely reflect their purpose. Negated `initializing` to `initialize_complete`.
Did some manual tests.
Repository: mesos
Description (updated)
-------
* Renamed `initialized` to `initialize_started`.
* Renamed `initializing` to `initialize_complete`.
* Removed the (2) condition, described below:
The initialization synchronization logic contains three conditions, which check:
1) Was `initialize` called and is it done?
2) Was `initialize` called and is it not done?
3) Are you the first to call `initialize`?
Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
Diffs (updated)
-----
3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
Diff: https://reviews.apache.org/r/39949/diff/
Testing (updated)
-------
`make check`
Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` with:
```
const size_t numThreads = 50;
std::thread* runningThreads[numThreads];
// Create additional threads.
for (size_t i = 0; i < numThreads; i++) {
runningThreads[i] = new std::thread([]() {
process::initialize();
});
}
for (size_t i = 0; i < numThreads; i++) {
runningThreads[i]->join();
delete runningThreads[i];
}
```
(Also added `#include <thread>` to the header).
Rebuilt `libprocess-tests` with the modification and ran it a few times.
`3rdparty/libprocess/libprocess-tests`
Thanks,
Joseph Wu
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Nov. 4, 2015, 11:25 a.m., Neil Conway wrote:
> > Can we test that more thoroughly than just "make check"? e.g., if there's a unit test that tries to enter this logic with multiple threads at once, running that with gtest_repeat=1000 would be nice.
>
> Joseph Wu wrote:
> There are tons of methods in libprocess that call `process::initialize` as a side-effect, but at the same time, the libprocess test suite starts out with an essentially race-free init call (See: https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/tests/main.cpp#L52). So any `--gtest_repeat` or `--gtest_shuffle` won't actually test the init code. (The master and agent also call init once on startup.)
>
> I'm not sure how valuable it will be to, say, to spawn a bunch of threads that call `process::initialize`. Do you have any suggestions?
>
> Neil Conway wrote:
> You could hackup tests/main.cpp::main() to test concurrent calls to process::initialize() -- i.e.,
>
> ```if (getenv("TEST_LIBPROCESS_INIT")) { for (int i = 0; i < 100; i++) { /* spawn thread */ process::initialize(); } } }```
Done. Nothing funky seemed to happen. (And if I add a print statement in the body of the init, it only gets printed once, as expected.)
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/#review105115
-----------------------------------------------------------
On Nov. 4, 2015, 12:24 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 12:24 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Renamed `initialized` to `initialize_started`.
> * Renamed `initializing` to `initialize_complete`.
> * Removed the (2) condition, described below:
>
> The initialization synchronization logic contains three conditions, which check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
>
> Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
>
> The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/39949/diff/
>
>
> Testing
> -------
>
> `make check`
>
> Replaced `process::initialize();` in `3rdparty/libprocess/src/tests/main.cpp` with:
> ```
>
> const size_t numThreads = 50;
>
> std::thread* runningThreads[numThreads];
>
> // Create additional threads.
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i] = new std::thread([]() {
> process::initialize();
> });
> }
>
> for (size_t i = 0; i < numThreads; i++) {
> runningThreads[i]->join();
> delete runningThreads[i];
> }
> ```
> (Also added `#include <thread>` to the header).
>
> Rebuilt `libprocess-tests` with the modification and ran it a few times.
> `3rdparty/libprocess/libprocess-tests`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Nov. 4, 2015, 11:25 a.m., Neil Conway wrote:
> > Can we test that more thoroughly than just "make check"? e.g., if there's a unit test that tries to enter this logic with multiple threads at once, running that with gtest_repeat=1000 would be nice.
There are tons of methods in libprocess that call `process::initialize` as a side-effect, but at the same time, the libprocess test suite starts out with an essentially race-free init call (See: https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/tests/main.cpp#L52). So any `--gtest_repeat` or `--gtest_shuffle` won't actually test the init code. (The master and agent also call init once on startup.)
I'm not sure how valuable it will be to, say, to spawn a bunch of threads that call `process::initialize`. Do you have any suggestions?
> On Nov. 4, 2015, 11:25 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/src/process.cpp, line 742
> > <https://reviews.apache.org/r/39949/diff/1/?file=1115784#file1115784line742>
> >
> > I wonder if it would be an improvement to rename these variables to reflect what they are used for more clearly.
> >
> > For example: maybe call them "initialize_started" and "initialize_complete" (and change the second so that it goes from false -> true instead of true -> false).
That sounds very reasonable. I'll rename (and see if anyone objects :).
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/#review105115
-----------------------------------------------------------
On Nov. 4, 2015, 10:58 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 10:58 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The initialization synchronization logic contains three conditions, which check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
>
> Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
>
> The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/39949/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Neil Conway <ne...@gmail.com>.
> On Nov. 4, 2015, 7:25 p.m., Neil Conway wrote:
> > Can we test that more thoroughly than just "make check"? e.g., if there's a unit test that tries to enter this logic with multiple threads at once, running that with gtest_repeat=1000 would be nice.
>
> Joseph Wu wrote:
> There are tons of methods in libprocess that call `process::initialize` as a side-effect, but at the same time, the libprocess test suite starts out with an essentially race-free init call (See: https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/tests/main.cpp#L52). So any `--gtest_repeat` or `--gtest_shuffle` won't actually test the init code. (The master and agent also call init once on startup.)
>
> I'm not sure how valuable it will be to, say, to spawn a bunch of threads that call `process::initialize`. Do you have any suggestions?
You could hackup tests/main.cpp::main() to test concurrent calls to process::initialize() -- i.e.,
```if (getenv("TEST_LIBPROCESS_INIT")) { for (int i = 0; i < 100; i++) { /* spawn thread */ process::initialize(); } } }```
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/#review105115
-----------------------------------------------------------
On Nov. 4, 2015, 6:58 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 6:58 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The initialization synchronization logic contains three conditions, which check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
>
> Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
>
> The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/39949/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 39949: Document and simplify libprocess
initialization synchronization logic.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39949/#review105115
-----------------------------------------------------------
Can we test that more thoroughly than just "make check"? e.g., if there's a unit test that tries to enter this logic with multiple threads at once, running that with gtest_repeat=1000 would be nice.
3rdparty/libprocess/src/process.cpp (line 742)
<https://reviews.apache.org/r/39949/#comment163522>
I wonder if it would be an improvement to rename these variables to reflect what they are used for more clearly.
For example: maybe call them "initialize_started" and "initialize_complete" (and change the second so that it goes from false -> true instead of true -> false).
3rdparty/libprocess/src/process.cpp (line 751)
<https://reviews.apache.org/r/39949/#comment163523>
process::initialize.
3rdparty/libprocess/src/process.cpp (line 759)
<https://reviews.apache.org/r/39949/#comment163521>
Can you clarify what "the first ones" means here?
Can
- Neil Conway
On Nov. 4, 2015, 6:58 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39949/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 6:58 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The initialization synchronization logic contains three conditions, which check:
> 1) Was `initialize` called and is it done?
> 2) Was `initialize` called and is it not done?
> 3) Are you the first to call `initialize`?
>
> Condition (3) uses `compare_exchange_strong` between `initialized` and `false`. This returns `true` (and sets `initialized` to true) iff the caller is the first to reach that expression.
>
> The second simultaneous caller of `initialize` will either satisify condition (2) or (3) and then wait on `initializing`. For the second caller, (2) and (3) are identical because `compare_exchange_strong` between `true` and `false` will always return false, thereby putting the second caller into the waiting loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/39949/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Joseph Wu
>
>