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#newcode180
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#newcode188
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.cc#newcode7436
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... ;-)
https://codereview.chromium.org/812583003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
