Great progress on this tough issue! I'll need one more quick pass after you address the comments.
Thanks, Vitaly http://codereview.chromium.org/2910004/diff/1/11 File src/isolate.cc (right): http://codereview.chromium.org/2910004/diff/1/11#newcode197 src/isolate.cc:197: static bool static_initialized = Isolate::EnsureStaticInitialized(); Why do you need this bool? http://codereview.chromium.org/2910004/diff/1/11#newcode254 src/isolate.cc:254: USE(static_initialized); What's the expected effect of this? http://codereview.chromium.org/2910004/diff/1/11#newcode257 src/isolate.cc:257: default_isolate_ = new_default_isolate; I'm afraid this doesn't make the double-checked-locking here safe. You need a memory barrier before assigning to "default_isolate_". Try using ReleaseStore. (See http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf) Or better yet rewrite this to always grab the mutex -- I don't think initialization is that performance critical. http://codereview.chromium.org/2910004/diff/1/12 File src/isolate.h (right): http://codereview.chromium.org/2910004/diff/1/12#newcode308 src/isolate.h:308: class ThreadDataTable { Can this be made private? http://codereview.chromium.org/2910004/diff/1/12#newcode333 src/isolate.h:333: prev_(NULL) { nit: Put '}' on this line. http://codereview.chromium.org/2910004/diff/1/12#newcode341 src/isolate.h:341: bool matches(Isolate* isolate, ThreadId thread_id) const { nit: "Matches" http://codereview.chromium.org/2910004/diff/1/12#newcode368 src/isolate.h:368: // Returns the PerThread record for the current thread (or NULL if one is "PerIsolateThreadData record"? http://codereview.chromium.org/2910004/diff/1/12#newcode371 src/isolate.h:371: PerIsolateThreadData* per_thread = reinterpret_cast<PerIsolateThreadData*>( nit: Having this local variable doesn't improve readability. Just return the value. http://codereview.chromium.org/2910004/diff/1/12#newcode376 src/isolate.h:376: // Returns the isolate inside which the current thread is running (or NULL The "or NULL" part doesn't really make sense given that we'll hit the assertion in this case. In release mode if NULL is somehow returned we'll crash almost immediately trying to dereference it. http://codereview.chromium.org/2910004/diff/1/12#newcode398 src/isolate.h:398: bool IsDefaultIsolate() { return this == default_isolate_; } Missing const. http://codereview.chromium.org/2910004/diff/1/12#newcode841 src/isolate.h:841: static Mutex* process_wide_mutex_; Please document the state this mutex guards (list of the fields). http://codereview.chromium.org/2910004/diff/1/14 File src/platform-linux.cc (right): http://codereview.chromium.org/2910004/diff/1/14#newcode747 src/platform-linux.cc:747: if (isolate == NULL) return false; This looks wrong (because of the assertion in Current()). Maybe we should expose UncheckedCurrent()? http://codereview.chromium.org/2910004/diff/1/14#newcode821 src/platform-linux.cc:821: : isolate_(isolate), interval_(interval), profiling_(profiling), nit: One initialize per line (when they don't all fit). http://codereview.chromium.org/2910004/diff/1/15 File src/platform-macos.cc (right): http://codereview.chromium.org/2910004/diff/1/15#newcode627 src/platform-macos.cc:627: : isolate_(isolate), interval_(interval), profiling_(profiling), Same nit as above. http://codereview.chromium.org/2910004/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
