Re: Review Request 39949: Document and simplify libprocess initialization synchronization logic.

[Joris Van Remoortere]

---
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 ` 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.

[Joseph Wu]

---
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 ` 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.

[Joseph Wu]

---
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 ` 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.

[Joseph Wu]

> On Nov. 7, 2015, 7:35 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 742-770
> > 
> >
> > 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 ` 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.

[Ben Mahler]

---
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)


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 ` 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.

[Mesos ReviewBot]

---
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 ` 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.

[Joseph Wu]

> 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 ` 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.

[Mesos ReviewBot]

---
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 ` 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.

[Joseph Wu]

---
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 ` 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.

[Neil Conway]

---
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 ` 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.

[Joseph Wu]

> 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 ` 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.

[Joseph Wu]

---
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 ` 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.

[Neil Conway]

> 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.

[Joseph Wu]

> 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
> > 
> >
> > 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.

[Neil Conway]

---
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)


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)


process::initialize.



3rdparty/libprocess/src/process.cpp (line 759)


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
> 
>

Review Request 39949: Document and simplify libprocess initialization synchronization logic.

[Joseph Wu]

---
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