Do not rely on uniquiness of pthread_t

Do not rely on uniquiness of pthread_t 

pthreads implementations are free to reuse pthread_t (thread id) after
the thread has died. This change gets rid of ThreadHandle class 
and replaces it with v8-managed thread identifiers.

BUG=
TEST=

[Dmitry Lomov, 2011-04-08 00:42:14]

Fallout from my work on lockers, in separate patch to make code reviews simple.
It turns out that pthread implementations reuse thread ids after the thread
dies, therefore ThreadHandle::IsSelf is unreliable, which made
ThreadManager->RestoreThread behave erroneously.

This change:
1) Gets rid of ThreadHandle class
2) Adds a new class ThreadRef, similiar in functionality to ThreadHandle, but
based on v8-maintained thread identifier that is put into the thread local. I
named the class ThreadRef instead of ThreadHandle since it is mutable (as the
ThreadHandle was before, but Handle meand something immutable, at least to me)

[Vitaly Repeshko, 2011-04-08 01:34:38]

Dmitry,

Thanks a lot for working on cleaning this up!

I've got a few high-level comments.

I don't really see the value in having both ThreadRef and ThreadId. To make
things simple could just have some purely static class with kInvalidId and
Current(), or we could make ThreadId a value class (using default copy ctor and
operator=) with an interface like:
class ThreadId {
 public:
  static ThreadId Current();
  static ThreadId Invalid();
  static ThreadId FromInteger(int);  // Is this really needed?

  bool Equals(const ThreadId& other);

  int ToInteger();  // ?
};

We avoid using the STL. We have our own List class that has std::vector
functionality.


-- Vitaly

[Dmitry Lomov, 2011-04-08 01:45:58]

Yes - sounds good - I shot for replacement of ThreadHandle as it was used
before, but value class sounds good.
On 2011/04/08 01:34:38, Vitaly Repeshko wrote:
> Dmitry,
> 
> Thanks a lot for working on cleaning this up!
> 
> I've got a few high-level comments.
> 
> I don't really see the value in having both ThreadRef and ThreadId. To make
> things simple could just have some purely static class with kInvalidId and
> Current(), or we could make ThreadId a value class (using default copy ctor
and
> operator=) with an interface like:
> class ThreadId {
>  public:
>   static ThreadId Current();
>   static ThreadId Invalid();
>   static ThreadId FromInteger(int);  // Is this really needed?
> 
>   bool Equals(const ThreadId& other);
> 
>   int ToInteger();  // ?
> };
> 
> We avoid using the STL. We have our own List class that has std::vector
> functionality.
> 
> 
> -- Vitaly

[Dmitry Lomov, 2011-04-08 04:10:27]

Code review feedback addressed (use value class throughout; still leaking the
integer ids though because of public API, but that is fine)

[Vitaly Repeshko, 2011-04-11 19:28:52]

LGTM with a few style nits and one real comment.

http://codereview.chromium.org/6816038/diff/5001/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/6816038/diff/5001/src/isolate.cc#newcode57
src/isolate.cc:57: Mutex* ThreadId::thread_id_process_wide_mutex_ =
OS::CreateMutex();
I'm worried that this could be accessed before the mutex is statically
initialized. I see two ways out: 1) add "static bool is_initialized = true;" and
check that it's true before entering thread id code (this will show if there're
any problematic usages) or 2) use atomic ops (Increment) and avoid using the
mutex completely. I'm in favor of (2).

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h
File src/isolate.h (right):

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode137
src/isolate.h:137: 
Style nit: two blank lines between top-level declarations.

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode138
src/isolate.h:138: // Platform-independent, reliable thread identifier
nit: Comments that are full sentences should start with a capital letter and end
with a period.

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode148
src/isolate.h:148: static ThreadId Invalid() { return ThreadId(kInvalidId); }
!thread_id.Equals(ThreadId::Invalid()) seems to be a common pattern. To improve
readability add IsValid() instance method.

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode164
src/isolate.h:164: int id_;
Please sort these following the rules in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode175
src/isolate.h:175: 
Two blank lines.

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode492
src/isolate.h:492: static Thread::LocalStorageKey thread_id_key() {
Restore the comment.

http://codereview.chromium.org/6816038/diff/5001/src/platform.h
File src/platform.h (right):

http://codereview.chromium.org/6816038/diff/5001/src/platform.h#newcode444
src/platform.h:444: 
nit: Delete one blank line.

http://codereview.chromium.org/6816038/diff/5001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6816038/diff/5001/src/runtime.cc#newcode10156
src/runtime.cc:10156: ThreadId::Current().ToInteger()));
Fits on the line above?

http://codereview.chromium.org/6816038/diff/5001/src/v8threads.cc
File src/v8threads.cc (right):

http://codereview.chromium.org/6816038/diff/5001/src/v8threads.cc#newcode144
src/v8threads.cc:144: 
Remove the newly added blank line.

http://codereview.chromium.org/6816038/diff/5001/src/v8threads.h
File src/v8threads.h (right):

http://codereview.chromium.org/6816038/diff/5001/src/v8threads.h#newcode86
src/v8threads.h:86: 
Only two-blank lines are needed here.

http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc
File test/cctest/test-threads.cc (right):

http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#...
test/cctest/test-threads.cc:144: i::List<i::ThreadId>& refs,
Non-const references are prohibited in parameters (see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...).

http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#...
test/cctest/test-threads.cc:145: unsigned int nThread,
noCamelCaseOrHungaryPlease!
use_std_lib_style_for_variables_and_certain_functions

http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#...
test/cctest/test-threads.cc:163: }
Weird indentation.

http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#...
test/cctest/test-threads.cc:189: delete threads[i];
Should we join a thread before deleting it?

[Dmitry Lomov, 2011-04-11 20:08:35]

Thanks for the review - I'll fix the style issues.

I have two comments on your comments.

http://codereview.chromium.org/6816038/diff/5001/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/6816038/diff/5001/src/isolate.cc#newcode57
src/isolate.cc:57: Mutex* ThreadId::thread_id_process_wide_mutex_ =
OS::CreateMutex();
I think we have the same issue with process_wide_mutex in Isolate class (see
below in this file). So I think this is benign. However atomics are cleaner and
faster - I'll change the impl  

On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> I'm worried that this could be accessed before the mutex is statically
> initialized. I see two ways out: 1) add "static bool is_initialized = true;"
and
> check that it's true before entering thread id code (this will show if
there're
> any problematic usages) or 2) use atomic ops (Increment) and avoid using the
> mutex completely. I'm in favor of (2).

http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc
File test/cctest/test-threads.cc (right):

http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#...
test/cctest/test-threads.cc:189: delete threads[i];
On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> Should we join a thread before deleting it?
NO! We must not. Thread::Join relies on pthread_t still referring to the same
thread, and this might not be the case if thread died

[Dmitry Lomov, 2011-04-11 21:41:14]

Style issues fixed + use atomic for highest_thread_id_

http://codereview.chromium.org/6816038/diff/5001/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/6816038/diff/5001/src/isolate.cc#newcode57
src/isolate.cc:57: Mutex* ThreadId::thread_id_process_wide_mutex_ =
OS::CreateMutex();
On 2011/04/11 20:08:35, dslomov wrote:
> I think we have the same issue with process_wide_mutex in Isolate class (see
> below in this file). So I think this is benign. However atomics are cleaner
and
> faster - I'll change the impl  
> 
> On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> > I'm worried that this could be accessed before the mutex is statically
> > initialized. I see two ways out: 1) add "static bool is_initialized = true;"
> and
> > check that it's true before entering thread id code (this will show if
> there're
> > any problematic usages) or 2) use atomic ops (Increment) and avoid using the
> > mutex completely. I'm in favor of (2).
> 

Done.

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h
File src/isolate.h (right):

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode137
src/isolate.h:137: 
On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> Style nit: two blank lines between top-level declarations.

Done.

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode138
src/isolate.h:138: // Platform-independent, reliable thread identifier
On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> nit: Comments that are full sentences should start with a capital letter and
end
> with a period.

Done.

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode148
src/isolate.h:148: static ThreadId Invalid() { return ThreadId(kInvalidId); }
On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> !thread_id.Equals(ThreadId::Invalid()) seems to be a common pattern. To
improve
> readability add IsValid() instance method.

Done.

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode164
src/isolate.h:164: int id_;
On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> Please sort these following the rules in
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode175
src/isolate.h:175: 
On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> Two blank lines.

Done.

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode492
src/isolate.h:492: static Thread::LocalStorageKey thread_id_key() {
On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> Restore the comment.

Done.

http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode492
src/isolate.h:492: static Thread::LocalStorageKey thread_id_key() {
On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> Restore the comment.

Done.

http://codereview.chromium.org/6816038/diff/5001/src/platform.h
File src/platform.h (right):

http://codereview.chromium.org/6816038/diff/5001/src/platform.h#newcode444
src/platform.h:444: 
On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> nit: Delete one blank line.

Done.

http://codereview.chromium.org/6816038/diff/5001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6816038/diff/5001/src/runtime.cc#newcode10156
src/runtime.cc:10156: ThreadId::Current().ToInteger()));
On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> Fits on the line above?

Done.

http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc
File test/cctest/test-threads.cc (right):

http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#...
test/cctest/test-threads.cc:144: i::List<i::ThreadId>& refs,
On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> Non-const references are prohibited in parameters (see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...).

Done.

http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#...
test/cctest/test-threads.cc:145: unsigned int nThread,
On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> noCamelCaseOrHungaryPlease!
> use_std_lib_style_for_variables_and_certain_functions

Done.

http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#...
test/cctest/test-threads.cc:163: }
On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
> Weird indentation.

Done.

[Vitaly Repeshko, 2011-04-11 21:48:29]

Still LGTM

[Dmitry Lomov, 2011-04-12 00:07:01]

Fix for Win32 build - I need help landing this as I do not have presmissions.