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#newcode189 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 http://codereview.chromium.org/6816038/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
