Author: [email protected]
Date: Wed Dec 17 09:40:02 2008
New Revision: 994
Added:
branches/bleeding_edge/test/cctest/test-threads.cc
Modified:
branches/bleeding_edge/src/v8threads.cc
branches/bleeding_edge/src/v8threads.h
branches/bleeding_edge/test/cctest/SConscript
Log:
Fix issue 142:
- Removed the potential for a NULL pointer access in
ContextSwitcher::PreemptionReceived.
- Removed a leak of the semaphore in the ContexSwitcher thread, by removing
the need for this semaphore entirely.
- Added a regression test case which will catch accesses to the
ContextSwitcher
singleton after it has been stopped.
Review URL: http://codereview.chromium.org/14483
Modified: branches/bleeding_edge/src/v8threads.cc
==============================================================================
--- branches/bleeding_edge/src/v8threads.cc (original)
+++ branches/bleeding_edge/src/v8threads.cc Wed Dec 17 09:40:02 2008
@@ -269,62 +269,64 @@
}
+// This is the ContextSwitcher singleton. There is at most a single thread
+// running which delivers preemption events to V8 threads.
+ContextSwitcher* ContextSwitcher::singleton_ = NULL;
+
+
ContextSwitcher::ContextSwitcher(int every_n_ms)
- : preemption_semaphore_(OS::CreateSemaphore(0)),
- keep_going_(true),
+ : keep_going_(true),
sleep_ms_(every_n_ms) {
}
-static v8::internal::ContextSwitcher* switcher;
-
-
+// Set the scheduling interval of V8 threads. This function starts the
+// ContextSwitcher thread if needed.
void ContextSwitcher::StartPreemption(int every_n_ms) {
ASSERT(Locker::IsLocked());
- if (switcher == NULL) {
- switcher = new ContextSwitcher(every_n_ms);
- switcher->Start();
+ if (singleton_ == NULL) {
+ // If the ContextSwitcher thread is not running at the moment start it
now.
+ singleton_ = new ContextSwitcher(every_n_ms);
+ singleton_->Start();
} else {
- switcher->sleep_ms_ = every_n_ms;
+ // ContextSwitcher thread is already running, so we just change the
+ // scheduling interval.
+ singleton_->sleep_ms_ = every_n_ms;
}
}
+// Disable preemption of V8 threads. If multiple threads want to use V8
they
+// must cooperatively schedule amongst them from this point on.
void ContextSwitcher::StopPreemption() {
ASSERT(Locker::IsLocked());
- if (switcher != NULL) {
- switcher->Stop();
- delete(switcher);
- switcher = NULL;
+ if (singleton_ != NULL) {
+ // The ContextSwitcher thread is running. We need to stop it and
release
+ // its resources.
+ singleton_->keep_going_ = false;
+ singleton_->Join(); // Wait for the ContextSwitcher thread to exit.
+ // Thread has exited, now we can delete it.
+ delete(singleton_);
+ singleton_ = NULL;
}
}
+// Main loop of the ContextSwitcher thread: Preempt the currently running
V8
+// thread at regular intervals.
void ContextSwitcher::Run() {
while (keep_going_) {
OS::Sleep(sleep_ms_);
StackGuard::Preempt();
- WaitForPreemption();
}
}
-void ContextSwitcher::Stop() {
- ASSERT(Locker::IsLocked());
- keep_going_ = false;
- preemption_semaphore_->Signal();
- Join();
-}
-
-
-void ContextSwitcher::WaitForPreemption() {
- preemption_semaphore_->Wait();
-}
-
-
+// Acknowledge the preemption by the receiving thread.
void ContextSwitcher::PreemptionReceived() {
ASSERT(Locker::IsLocked());
- switcher->preemption_semaphore_->Signal();
+ // There is currently no accounting being done for this. But could be in
the
+ // future, which is why we leave this in.
}
Modified: branches/bleeding_edge/src/v8threads.h
==============================================================================
--- branches/bleeding_edge/src/v8threads.h (original)
+++ branches/bleeding_edge/src/v8threads.h Wed Dec 17 09:40:02 2008
@@ -87,19 +87,31 @@
};
+// The ContextSwitcher thread is used to schedule regular preemptions to
+// multiple running V8 threads. Generally it is necessary to call
+// StartPreemption if there is more than one thread running. If not, a
single
+// JavaScript can take full control of V8 and not allow other threads to
run.
class ContextSwitcher: public Thread {
public:
- void Run();
+ // Set the preemption interval for the ContextSwitcher thread.
static void StartPreemption(int every_n_ms);
+
+ // Stop sending preemption requests to threads.
static void StopPreemption();
+
+ // Preempted thread needs to call back to the ContextSwitcher to
acknowlege
+ // the handling of a preemption request.
static void PreemptionReceived();
+
private:
explicit ContextSwitcher(int every_n_ms);
- void WaitForPreemption();
- void Stop();
- Semaphore* preemption_semaphore_;
+
+ void Run();
+
bool keep_going_;
int sleep_ms_;
+
+ static ContextSwitcher* singleton_;
};
} } // namespace v8::internal
Modified: branches/bleeding_edge/test/cctest/SConscript
==============================================================================
--- branches/bleeding_edge/test/cctest/SConscript (original)
+++ branches/bleeding_edge/test/cctest/SConscript Wed Dec 17 09:40:02 2008
@@ -38,7 +38,7 @@
'test-ast.cc', 'test-heap.cc', 'test-utils.cc', 'test-compiler.cc',
'test-spaces.cc', 'test-mark-compact.cc', 'test-lock.cc',
'test-conversions.cc', 'test-strings.cc', 'test-serialize.cc',
- 'test-decls.cc', 'test-alloc.cc', 'test-regexp.cc'
+ 'test-decls.cc', 'test-alloc.cc', 'test-regexp.cc', 'test-threads.cc'
],
'arch:arm': ['test-assembler-arm.cc', 'test-disasm-arm.cc'],
'arch:ia32': ['test-assembler-ia32.cc', 'test-disasm-ia32.cc'],
Added: branches/bleeding_edge/test/cctest/test-threads.cc
==============================================================================
--- (empty file)
+++ branches/bleeding_edge/test/cctest/test-threads.cc Wed Dec 17 09:40:02
2008
@@ -0,0 +1,54 @@
+// Copyright 2008 the V8 project authors. All rights reserved.
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+// * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+// * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following
+// disclaimer in the documentation and/or other materials provided
+// with the distribution.
+// * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived
+// from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+#include "v8.h"
+
+#include "platform.h"
+
+#include "cctest.h"
+
+
+TEST(Preemption) {
+ v8::Locker locker;
+ v8::V8::Initialize();
+ v8::HandleScope scope;
+ v8::Context::Scope context_scope(v8::Context::New());
+
+ v8::Locker::StartPreemption(100);
+
+ v8::Handle<v8::Script> script = v8::Script::Compile(
+ v8::String::New("var count = 0; var obj = new Object();
count++;\n"));
+
+ script->Run();
+
+ v8::Locker::StopPreemption();
+ v8::internal::OS::Sleep(500); // Make sure the timer fires.
+
+ script->Run();
+}
+
+
--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---
