http://codereview.chromium.org/174056/diff/1/5 File include/v8.h (right):
http://codereview.chromium.org/174056/diff/1/5#newcode2229 Line 2229: * The thread id for a thread should be retrieved immediately after On 2009/08/19 13:19:54, Christian Plesner Hansen wrote: > You should be more specific -- what does "immediately after" mean and what > happens if you do it at other times? Right. You just have to hold the V8 lock with that thread when getting its thread id. Documentation updated. http://codereview.chromium.org/174056/diff/1/5#newcode2262 Line 2262: static void TerminateExecution(); On 2009/08/19 13:19:54, Christian Plesner Hansen wrote: > Should the current thread bail out after calling this function as if > TerminateExecution had been called by a different thread? You can do anything you want after calling TerminateExecution. The next time JavaScript is entered a termination exception will be thrown (at the first stack guard check). http://codereview.chromium.org/174056/diff/1/5#newcode2327 Line 2327: bool IsTerminationException() const; On 2009/08/19 13:19:54, Christian Plesner Hansen wrote: > I'm wondering: maybe the it would be more obvious how to use this if there was a > more abstract "CanContinue" method (or some other name) that covered any > situation where applications have to bail out. That way we can adapt it if we > ever decide to add other cases -- like out of memory -- without requiring people > to change their code. That makes sense, thanks. I have replaced it by a CanContinue method. http://codereview.chromium.org/174056/diff/1/6 File src/api.cc (right): http://codereview.chromium.org/174056/diff/1/6#newcode3365 Line 3365: return i::Top::thread_id(); On 2009/08/19 13:19:54, Christian Plesner Hansen wrote: > Consider adding an API_ENTRY_CHECK to ensure that a Locker has been acquired > before this method is called. Yes, done. http://codereview.chromium.org/174056/diff/1/6#newcode3371 Line 3371: // If the thread_id identifies the current thread just terminate On 2009/08/19 13:19:54, Christian Plesner Hansen wrote: > Ditto API_ENTRY_CHECK. Yes, done. http://codereview.chromium.org/174056/diff/1/6#newcode3384 Line 3384: i::StackGuard::TerminateExecution(); On 2009/08/19 13:19:54, Christian Plesner Hansen wrote: > Ditto API_ENTRY_CHECK. This one does not need it since messing with the stack guards are protected internally in V8. This version of TerminateExecution can be called from any thread without holding the V8 lock. I have updated the documentation for this method with that information. http://codereview.chromium.org/174056/diff/1/17 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/174056/diff/1/17#newcode5845 Line 5845: // Special handling of out of memory excpetions. On 2009/08/19 13:19:54, Christian Plesner Hansen wrote: > Typo, excpetions. Done. http://codereview.chromium.org/174056/diff/1/17#newcode5857 Line 5857: // Special handling of termination exceptions which are uncatchable On 2009/08/19 13:19:54, Christian Plesner Hansen wrote: > It would be nice if handling of termination and OOM could share more code. But > I understand if you don't want to touch OOM at this point. Right, this change is complicated as is and I didn't want to mess with the OOM handling at the same time. http://codereview.chromium.org/174056/diff/1/20 File src/execution.cc (right): http://codereview.chromium.org/174056/diff/1/20#newcode657 Line 657: return Top::TerminateExecution(); On 2009/08/19 14:53:35, Lasse Reichstein wrote: > It should work. Top::TerminateExecution() returns Failure::Exception, which is > what the native RegExp checks against (result->IsException). It will be handled > just as any other exception by the RegExp calling code, which means terminating > the regexp and returning Failure::Exception() from the runtime call. Thanks for the clearification Lasse - I was not sure. :) http://codereview.chromium.org/174056/diff/1/8 File src/top.cc (right): http://codereview.chromium.org/174056/diff/1/8#newcode724 Line 724: !catchable_by_javascript); On 2009/08/19 13:19:54, Christian Plesner Hansen wrote: > Why not do (a ||b || c) instead of ((a || b) || c) ? Done. http://codereview.chromium.org/174056/diff/1/8#newcode749 Line 749: ShouldReportException(&is_caught_externally, !is_termination_exception); On 2009/08/19 13:19:54, Christian Plesner Hansen wrote: > Shouldn't is_out_of_memory imply !catchable_by_javascript? If that is the case > then maybe define catchable_by_javascript locally as (!is_out_of_memory || > !is_termination_exception) and use it the three places below where that > expression is used. Done. http://codereview.chromium.org/174056/diff/1/8#newcode751 Line 751: !is_out_of_memory && !is_termination_exception && should_return_exception; On 2009/08/19 13:19:54, Christian Plesner Hansen wrote: > This code has become messy -- ShouldReportException no longer calculates if the > message should be reported which is confusing. Maybe rename it > ShouldReturnException to match how it is used. Done. http://codereview.chromium.org/174056/diff/1/13 File src/v8threads.cc (right): http://codereview.chromium.org/174056/diff/1/13#newcode324 Line 324: if (!Thread::HasThreadLocal(thread_id_key)) { On 2009/08/19 13:19:54, Christian Plesner Hansen wrote: > I would suggest adding an ASSERT(Locker::IsLocked()) here, otherwise the > next_id++ looks really suspect. I have added the assertion. Right now, AssignId is only called from the Locker constructor itself after the V8 lock has been successfully acquired. However, adding the assert makes sense. http://codereview.chromium.org/174056/diff/1/3 File test/cctest/test-thread-termination.cc (right): http://codereview.chromium.org/174056/diff/1/3#newcode124 Line 124: global->Set(v8::String::New("terminate"), v8::FunctionTemplate::New(Signal)); On 2009/08/19 13:19:54, Christian Plesner Hansen wrote: > This code occurs (almost) three times. Maybe factor it out into a separate > function. Done. http://codereview.chromium.org/174056 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
