Re: Review Request 37877: libprocess: Replace GCC instrinsics and volatile with std::atomic.

2015-09-10 Thread Neil Conway 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37877/
---

(Updated Sept. 10, 2015, 6:54 p.m.)


Review request for mesos, Joris Van Remoortere and switched to 'mcypark'.


Bugs: MESOS-3326
https://issues.apache.org/jira/browse/MESOS-3326


Repository: mesos


Description
---

MESOS-3326.


Diffs (updated)
-

  3rdparty/libprocess/include/process/latch.hpp 
a1a2227a9edcc31fd82c6410262aa4565fd66cb2 
  3rdparty/libprocess/include/process/metrics/counter.hpp 
e51a8beb80b15dd64aa2e481036ae8ba37125640 
  3rdparty/libprocess/include/process/process.hpp 
cc8317f1bc25bfa1be207a3e212b8cfc75248404 
  3rdparty/libprocess/src/clock.cpp 09c60e5a5d9bd9fc5511e57f3209fad7dbf834d6 
  3rdparty/libprocess/src/latch.cpp f7d94d92e85c58878d98e13757b6fc37837ca977 
  3rdparty/libprocess/src/process.cpp 0e5394acff16376809918d583d7aee582cc6da54 
  3rdparty/libprocess/src/process_reference.hpp 
f8df4a6dcf01bb7af750c1ed9e85c64cea2042c5 

Diff: https://reviews.apache.org/r/37877/diff/


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 37877: libprocess: Replace GCC instrinsics and volatile with std::atomic.

2015-09-10 Thread Joris Van Remoortere 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37877/#review98522
---



3rdparty/libprocess/src/latch.cpp (line 41)


renaming these to `expected`



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


substituting the triple backticks to single backticks. sorry for the 
confusion.


- Joris Van Remoortere


On Sept. 10, 2015, 6:54 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37877/
> ---
> 
> (Updated Sept. 10, 2015, 6:54 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3326
> https://issues.apache.org/jira/browse/MESOS-3326
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3326.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/latch.hpp 
> a1a2227a9edcc31fd82c6410262aa4565fd66cb2 
>   3rdparty/libprocess/include/process/metrics/counter.hpp 
> e51a8beb80b15dd64aa2e481036ae8ba37125640 
>   3rdparty/libprocess/include/process/process.hpp 
> cc8317f1bc25bfa1be207a3e212b8cfc75248404 
>   3rdparty/libprocess/src/clock.cpp 09c60e5a5d9bd9fc5511e57f3209fad7dbf834d6 
>   3rdparty/libprocess/src/latch.cpp f7d94d92e85c58878d98e13757b6fc37837ca977 
>   3rdparty/libprocess/src/process.cpp 
> 0e5394acff16376809918d583d7aee582cc6da54 
>   3rdparty/libprocess/src/process_reference.hpp 
> f8df4a6dcf01bb7af750c1ed9e85c64cea2042c5 
> 
> Diff: https://reviews.apache.org/r/37877/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37877: libprocess: Replace GCC instrinsics and volatile with std::atomic.

2015-09-09 Thread Joris Van Remoortere 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37877/#review98207
---

Ship it!



3rdparty/libprocess/include/process/metrics/counter.hpp (line 78)


Since we're touching this, can we use a more meaningful variable name? We 
use full words for variable names in Mesos. (Not yours, I know ;-))

Also pointing out the explicit specialization mentioned in a prior review.



3rdparty/libprocess/include/process/process.hpp (line 335)


Is this a place where we want to change the code to use a more suitable 
type? My thoughts are "If we're reference counting, we should be unsigned" and 
"If we're on a 64-bit architecture we might as well use a long to avoid chances 
of overflowing the ref count".

Thoughts?

We could do this in a subsequent review to separate the refactorings.



3rdparty/libprocess/src/latch.cpp (lines 41 - 44)


Please use full words for variable names.

Similar below.



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


Another one where I'm curious if it would be worth increasing the range 
(and possibly only expressing valid values, >=0) for safety.



3rdparty/libprocess/src/process.cpp (lines 757 - 773)


Would you agree that what is happening here is not immediately obvious?

Now that you understand it, would you mind adding a few comments around the 
different exit conditions and which stage of intitialization they represent?

In particular I think the exit condition around 769 could use a comment.



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


Can you wrap compare_exchange_strong in backticks ```



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


Full variable names.



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


let's use backticks ```



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


nice catch.



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


nice catch.


- Joris Van Remoortere


On Sept. 9, 2015, 4:01 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37877/
> ---
> 
> (Updated Sept. 9, 2015, 4:01 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3326
> https://issues.apache.org/jira/browse/MESOS-3326
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3326.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/latch.hpp 
> a1a2227a9edcc31fd82c6410262aa4565fd66cb2 
>   3rdparty/libprocess/include/process/metrics/counter.hpp 
> e51a8beb80b15dd64aa2e481036ae8ba37125640 
>   3rdparty/libprocess/include/process/process.hpp 
> 009f7c4167fa379ac6b1c267e1b4d5fcdf28d697 
>   3rdparty/libprocess/src/clock.cpp 09c60e5a5d9bd9fc5511e57f3209fad7dbf834d6 
>   3rdparty/libprocess/src/latch.cpp f7d94d92e85c58878d98e13757b6fc37837ca977 
>   3rdparty/libprocess/src/process.cpp 
> 755187c8761137cb2bf2f7295b29a63f63c68bc6 
>   3rdparty/libprocess/src/process_reference.hpp 
> f8df4a6dcf01bb7af750c1ed9e85c64cea2042c5 
> 
> Diff: https://reviews.apache.org/r/37877/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37877: libprocess: Replace GCC instrinsics and volatile with std::atomic.

2015-09-09 Thread Neil Conway 


> On Sept. 9, 2015, 5:16 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 757-773
> > 
> >
> > Would you agree that what is happening here is not immediately obvious?
> > 
> > Now that you understand it, would you mind adding a few comments around 
> > the different exit conditions and which stage of intitialization they 
> > represent?
> > 
> > In particular I think the exit condition around 769 could use a comment.

"Not immediately obvious" is an understatement :)

I can certainly add comments, although I wonder whether this logic can't be 
rethought and simplified; if we need to keep the existing logic, seems we 
should point out the assignment to "initializing" down below.

I'll take a look at this, but can we do it as a separate review?


- Neil


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37877/#review98207
---


On Sept. 9, 2015, 4:01 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37877/
> ---
> 
> (Updated Sept. 9, 2015, 4:01 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3326
> https://issues.apache.org/jira/browse/MESOS-3326
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3326.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/latch.hpp 
> a1a2227a9edcc31fd82c6410262aa4565fd66cb2 
>   3rdparty/libprocess/include/process/metrics/counter.hpp 
> e51a8beb80b15dd64aa2e481036ae8ba37125640 
>   3rdparty/libprocess/include/process/process.hpp 
> 009f7c4167fa379ac6b1c267e1b4d5fcdf28d697 
>   3rdparty/libprocess/src/clock.cpp 09c60e5a5d9bd9fc5511e57f3209fad7dbf834d6 
>   3rdparty/libprocess/src/latch.cpp f7d94d92e85c58878d98e13757b6fc37837ca977 
>   3rdparty/libprocess/src/process.cpp 
> 755187c8761137cb2bf2f7295b29a63f63c68bc6 
>   3rdparty/libprocess/src/process_reference.hpp 
> f8df4a6dcf01bb7af750c1ed9e85c64cea2042c5 
> 
> Diff: https://reviews.apache.org/r/37877/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>