Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
812583003:
Support tasks injection into a running VM. (Closed)
DescriptionSupport tasks injection into a running VM.
The new mechanism unlike DebugCommands will run even when the debugger is not active. In other words it doesn't require setting DebugEventListener.
One of supposed usages is an ability to start profiling while JS is running a tight loop.
Slightly tweaked patch from yurys@
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressing comments. #
Total comments: 1
Patch Set 3 : Added a comment. #
Total comments: 16
Patch Set 4 : Addressing comments. #
Total comments: 8
Patch Set 5 : Addressing comments. #
Messages
Total messages: 23 (7 generated)
alph@chromium.org changed reviewers: + loislo@chromium.org, yangguo@chromium.org, yurys@chromium.org
Folks, please take a look.
Please add into the description that the new mechanism unlike DebugCommands will run when debugger is not active. In other words it doesn't require setting DebugEventListener. https://codereview.chromium.org/812583003/diff/1/include/v8-debug.h File include/v8-debug.h (right): https://codereview.chromium.org/812583003/diff/1/include/v8-debug.h#newcode188 include/v8-debug.h:188: // VM thread. The task is deleted after execution. Please update the comment to reflect that the task will be deleted regardless of the execution. https://codereview.chromium.org/812583003/diff/1/src/debug.cc File src/debug.cc (right): https://codereview.chromium.org/812583003/diff/1/src/debug.cc#newcode3110 src/debug.cc:3110: if (!isolate_->debug()->in_debug_scope()) { if (!in_debug_scope()) https://codereview.chromium.org/812583003/diff/1/src/debug.cc#newcode3153 src/debug.cc:3153: isolate_->stack_guard()->ClearDebugCommand(); It seems fine to reuse the same interrupt bit as for debug commands but we might want to reconsider it later. https://codereview.chromium.org/812583003/diff/1/src/debug.cc#newcode3535 src/debug.cc:3535: } empty line? https://codereview.chromium.org/812583003/diff/1/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/812583003/diff/1/test/cctest/test-debug.cc#ne... test/cctest/test-debug.cc:7407: v8::base::OS::Sleep(delay_ms_); We can avoid this Sleep. E.g. "while" loop below can call a native function that would on first call start this thread and after that would return false until StopExecutionTask is executed and flips some flag. https://codereview.chromium.org/812583003/diff/1/test/cctest/test-debug.cc#ne... test/cctest/test-debug.cc:7408: v8::Debug::BreakAndRun(isolate_, task_); task_ = NULL? https://codereview.chromium.org/812583003/diff/1/test/cctest/test-debug.cc#ne... test/cctest/test-debug.cc:7422: context_->Global()->Set(v8_str("stop"), v8::True(context_->GetIsolate())); We can use a static C++ variable instead, it would indicate that we can break the loop. https://codereview.chromium.org/812583003/diff/1/test/cctest/test-debug.cc#ne... test/cctest/test-debug.cc:7435: DelayedTask delayed_task(env->GetIsolate(), task, 100); Or can we at least reduce the delay to 10 or something?
https://codereview.chromium.org/812583003/diff/1/include/v8-debug.h File include/v8-debug.h (right): https://codereview.chromium.org/812583003/diff/1/include/v8-debug.h#newcode188 include/v8-debug.h:188: // VM thread. The task is deleted after execution. On 2014/12/17 14:25:02, yurys wrote: > Please update the comment to reflect that the task will be deleted regardless of > the execution. Done. https://codereview.chromium.org/812583003/diff/1/src/debug.cc File src/debug.cc (right): https://codereview.chromium.org/812583003/diff/1/src/debug.cc#newcode3110 src/debug.cc:3110: if (!isolate_->debug()->in_debug_scope()) { On 2014/12/17 14:25:02, yurys wrote: > if (!in_debug_scope()) Done. https://codereview.chromium.org/812583003/diff/1/src/debug.cc#newcode3153 src/debug.cc:3153: isolate_->stack_guard()->ClearDebugCommand(); On 2014/12/17 14:25:02, yurys wrote: > It seems fine to reuse the same interrupt bit as for debug commands but we might > want to reconsider it later. Acknowledged. https://codereview.chromium.org/812583003/diff/1/src/debug.cc#newcode3535 src/debug.cc:3535: } On 2014/12/17 14:25:02, yurys wrote: > empty line? Surprisingly git cl format does not want an empty line here. https://codereview.chromium.org/812583003/diff/1/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/812583003/diff/1/test/cctest/test-debug.cc#ne... test/cctest/test-debug.cc:7407: v8::base::OS::Sleep(delay_ms_); On 2014/12/17 14:25:03, yurys wrote: > We can avoid this Sleep. E.g. "while" loop below can call a native function that > would on first call start this thread and after that would return false until > StopExecutionTask is executed and flips some flag. I would like to make sure the BreakAndRun is engaged when the VM loops in a pure JS code (i.e. having no native callbacks). I realize Sleep does not guarantee that, but it makes the signal fall into random points of VM execution, i.e. it covers more cases. The test should not fail in any case, so it is not a source of flakiness. https://codereview.chromium.org/812583003/diff/1/test/cctest/test-debug.cc#ne... test/cctest/test-debug.cc:7408: v8::Debug::BreakAndRun(isolate_, task_); On 2014/12/17 14:25:03, yurys wrote: > task_ = NULL? Done. https://codereview.chromium.org/812583003/diff/1/test/cctest/test-debug.cc#ne... test/cctest/test-debug.cc:7422: context_->Global()->Set(v8_str("stop"), v8::True(context_->GetIsolate())); On 2014/12/17 14:25:03, yurys wrote: > We can use a static C++ variable instead, it would indicate that we can break > the loop. See my comment above. I'd like it to be a pure JS loop. https://codereview.chromium.org/812583003/diff/1/test/cctest/test-debug.cc#ne... test/cctest/test-debug.cc:7435: DelayedTask delayed_task(env->GetIsolate(), task, 100); On 2014/12/17 14:25:03, yurys wrote: > Or can we at least reduce the delay to 10 or something? Acknowledged.
lgtm https://codereview.chromium.org/812583003/diff/20001/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/812583003/diff/20001/test/cctest/test-debug.c... test/cctest/test-debug.cc:7436: DelayedTask delayed_task(env->GetIsolate(), task, 10); Please add a comment on why we need the delay.
alph@chromium.org changed reviewers: + bmeurer@chromium.org
Benedikt, could you please take a look. Thanks!
bmeurer@chromium.org changed reviewers: + dcarney@chromium.org, jochen@chromium.org, svenpanne@chromium.org
I think it's OK in general, but the debug api is not my area of expertise... CC'ing available experts. https://codereview.chromium.org/812583003/diff/40001/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/812583003/diff/40001/test/cctest/test-debug.c... test/cctest/test-debug.cc:7436: // Delaying the task increases probability the task is injected while the This is rather fragile. Can we have a more robust test case?
Hmmm, the overall approach looks a bit... adventurous: Calling arbitrary C++ code in the context of the running VM can be dangerous. Mutating the VM state then might trigger totally new untested code paths. https://codereview.chromium.org/812583003/diff/40001/include/v8-debug.h File include/v8-debug.h (right): https://codereview.chromium.org/812583003/diff/40001/include/v8-debug.h#newco... include/v8-debug.h:180: class Task { We already have v8::Task, why do we need another one? In general, it's a pity that we can't use std::function yet. https://codereview.chromium.org/812583003/diff/40001/include/v8-debug.h#newco... include/v8-debug.h:188: // VM thread. The VM takes ownership of the task. Please extend the comment a bit, describing that the task is run at most once and deleted after that. Another thing which needs a description: If it is not run, when exactly does it get destroyed? https://codereview.chromium.org/812583003/diff/40001/src/debug.h File src/debug.h (right): https://codereview.chromium.org/812583003/diff/40001/src/debug.h#newcode328 src/debug.h:328: bool IsEmpty() const; Not related to this CL, but anyway: Why is it safe to split Dequeue into IsEmpty/Get here? Looking at the call sites, it's far from clear that there can't be a race condition. https://codereview.chromium.org/812583003/diff/40001/src/debug.h#newcode340 src/debug.h:340: class LockingTaskQueue BASE_EMBEDDED { Remove BASE_EMBEDDED, it's nonsense, anyway... https://codereview.chromium.org/812583003/diff/40001/src/debug.h#newcode350 src/debug.h:350: mutable base::Mutex mutex_; Without having a single const function, why do we need mutable? https://codereview.chromium.org/812583003/diff/40001/src/debug.h#newcode351 src/debug.h:351: DISALLOW_COPY_AND_ASSIGN(LockingTaskQueue); Strictly speaking this is not necessary and just noise, Mutex itself can't be copied/assigned already. https://codereview.chromium.org/812583003/diff/40001/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/812583003/diff/40001/test/cctest/test-debug.c... test/cctest/test-debug.cc:7436: // Delaying the task increases probability the task is injected while the On 2014/12/18 04:11:57, Benedikt Meurer wrote: > This is rather fragile. Can we have a more robust test case? I would even go a step further: Without proper synchronization, this is not LGTM. We don't want to have "CPUProfiler reloaded" with all those flakes on the bots... ;-)
I'd recommend to extend the RequestInterrupt() API to support more than one callback for the use case you described (starting the profiler while JS runs in a tight loop)
@svenpanne We already allow execution of "arbitrary C++ code" on VM thread with SetDebugEventListener API. https://codereview.chromium.org/812583003/diff/40001/include/v8-debug.h File include/v8-debug.h (right): https://codereview.chromium.org/812583003/diff/40001/include/v8-debug.h#newco... include/v8-debug.h:180: class Task { On 2014/12/18 08:27:50, Sven Panne wrote: > We already have v8::Task, why do we need another one? > > In general, it's a pity that we can't use std::function yet. Sorry, missed the v8::Task. https://codereview.chromium.org/812583003/diff/40001/include/v8-debug.h#newco... include/v8-debug.h:188: // VM thread. The VM takes ownership of the task. On 2014/12/18 08:27:51, Sven Panne wrote: > Please extend the comment a bit, describing that the task is run at most once > and deleted after that. Another thing which needs a description: If it is not > run, when exactly does it get destroyed? Done. https://codereview.chromium.org/812583003/diff/40001/src/debug.h File src/debug.h (right): https://codereview.chromium.org/812583003/diff/40001/src/debug.h#newcode328 src/debug.h:328: bool IsEmpty() const; On 2014/12/18 08:27:51, Sven Panne wrote: > Not related to this CL, but anyway: Why is it safe to split Dequeue into > IsEmpty/Get here? Looking at the call sites, it's far from clear that there > can't be a race condition. While it's not related to this CL ;-), I think it is safe provided e.g. IsEmpty/Get is used on the consumer side only. In other words once IsEmpty returns false it keeps that state until Get is called. https://codereview.chromium.org/812583003/diff/40001/src/debug.h#newcode340 src/debug.h:340: class LockingTaskQueue BASE_EMBEDDED { On 2014/12/18 08:27:51, Sven Panne wrote: > Remove BASE_EMBEDDED, it's nonsense, anyway... Done. https://codereview.chromium.org/812583003/diff/40001/src/debug.h#newcode350 src/debug.h:350: mutable base::Mutex mutex_; On 2014/12/18 08:27:51, Sven Panne wrote: > Without having a single const function, why do we need mutable? We don't. Removed. https://codereview.chromium.org/812583003/diff/40001/src/debug.h#newcode351 src/debug.h:351: DISALLOW_COPY_AND_ASSIGN(LockingTaskQueue); On 2014/12/18 08:27:51, Sven Panne wrote: > Strictly speaking this is not necessary and just noise, Mutex itself can't be > copied/assigned already. I don't want it to be depended on the set of fields. E.g. a field may change at some point. https://codereview.chromium.org/812583003/diff/40001/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/812583003/diff/40001/test/cctest/test-debug.c... test/cctest/test-debug.cc:7436: // Delaying the task increases probability the task is injected while the On 2014/12/18 04:11:57, Benedikt Meurer wrote: > This is rather fragile. Can we have a more robust test case? This test case it robust. It does not fail/flake depending on the timeout value. By adding the timeout I just wanted to increase the probability the task is fired when VM is already running a pure-JS tight loop. However, as Yury pointed out in an offline chat, the purpose of this test is not to check the interrupts work for tight loops. But to check that the task gets executed. So I remove the Sleep once you insist. https://codereview.chromium.org/812583003/diff/40001/test/cctest/test-debug.c... test/cctest/test-debug.cc:7436: // Delaying the task increases probability the task is injected while the On 2014/12/18 08:27:51, Sven Panne wrote: > On 2014/12/18 04:11:57, Benedikt Meurer wrote: > > This is rather fragile. Can we have a more robust test case? > > I would even go a step further: Without proper synchronization, this is not > LGTM. We don't want to have "CPUProfiler reloaded" with all those flakes on the > bots... ;-) Again, this Sleep does not lead to flakiness. There's no race. Instead it made a better test coverage of time moments the task is injected. Anyway I removed it, once it's so frightening.
lgtm https://codereview.chromium.org/812583003/diff/60001/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/812583003/diff/60001/test/cctest/test-debug.c... test/cctest/test-debug.cc:7403: void Run() override { Use OVERRIDE, not all our toolchains support override yet. https://codereview.chromium.org/812583003/diff/60001/test/cctest/test-debug.c... test/cctest/test-debug.cc:7417: void Run() override { Same here. https://codereview.chromium.org/812583003/diff/60001/test/cctest/test-debug.c... test/cctest/test-debug.cc:7432: // loop is already running. Remove the comment, there's no delay anymore. https://codereview.chromium.org/812583003/diff/60001/test/cctest/test-debug.c... test/cctest/test-debug.cc:7436: CompileRun("while (!stop) {}"); Well, without any kind of handshake (via a native function), the test itself might not be flaky, but the thing it tests is. In a nutshell: Our test coverage is flaky now, but if that's OK for you...
https://codereview.chromium.org/812583003/diff/60001/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/812583003/diff/60001/test/cctest/test-debug.c... test/cctest/test-debug.cc:7403: void Run() override { On 2014/12/18 14:53:31, Sven Panne wrote: > Use OVERRIDE, not all our toolchains support override yet. Done. https://codereview.chromium.org/812583003/diff/60001/test/cctest/test-debug.c... test/cctest/test-debug.cc:7417: void Run() override { On 2014/12/18 14:53:31, Sven Panne wrote: > Same here. Done. https://codereview.chromium.org/812583003/diff/60001/test/cctest/test-debug.c... test/cctest/test-debug.cc:7432: // loop is already running. On 2014/12/18 14:53:31, Sven Panne wrote: > Remove the comment, there's no delay anymore. Done. https://codereview.chromium.org/812583003/diff/60001/test/cctest/test-debug.c... test/cctest/test-debug.cc:7436: CompileRun("while (!stop) {}"); On 2014/12/18 14:53:31, Sven Panne wrote: > Well, without any kind of handshake (via a native function), the test itself > might not be flaky, but the thing it tests is. In a nutshell: Our test coverage > is flaky now, but if that's OK for you... Technically the test doesn't care what was the VM state when the task was injected. It tests that the task is injected from another thread, and the task is executed. So I think the synchronization is not needed here.
On 2014/12/18 12:27:37, jochen (slow) wrote: > I'd recommend to extend the RequestInterrupt() API to support more than one > callback for the use case you described (starting the profiler while JS runs in > a tight loop) I don't see how to extend the API namely ClearInterrupt without breaking the backward compatibility.
The CQ bit was checked by alph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/812583003/80001
I'd just add a new API for the existing mechanism where the RequestInterrupt() call returns an id and the Clear call takes the ID. The old API can then be implemented on top of this... I think that's preferable over having two ways to achieve the same thing.
The CQ bit was unchecked by alph@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_arm64_rel on tryserver.v8 (http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1843)
On 2014/12/18 15:38:48, jochen (slow) wrote: > I'd just add a new API for the existing mechanism where the RequestInterrupt() > call returns an id and the Clear call takes the ID. The old API can then be > implemented on top of this... > > I think that's preferable over having two ways to achieve the same thing. I'm going to try this way. Thank you. Let it be QueueInterruptRequest and CancelInterruptRequest.
bmeurer@chromium.org changed reviewers: - bmeurer@chromium.org |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
