Block/resume primitive [v8/v8 : main]
Maksim Ivanov (Gerrit)
Maksim Ivanov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Maksim Ivanov
| Commit-Queue | +0 |
1 comment
Still not fully polished, but sharing for initial feedback. Using a GN flag guard to avoid having to think about performance in cross-thread syncing logic.
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 2 comments![]( "Open in Gerrit")
Will look at this in more detail in a bit.
} // namespace internalFood for thought: IIRC we also had issues where the main thread needed to block "some time". Can we have a similar helper that for `BlockForAt(timeout, synchronziation point)` that schedules a background task which unblock the point before it goes to sleep?
Related details
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Maksim Ivanov
| Commit-Queue | +1 |
2 comments
[testing] Block/resume primitiveI've checked that this API works to craft a straightforward POC for crbug.com/514446876. And that adding one more `SYNCHRONIZATION_POINT_FOR_TESTING` gives a reliable POC for crbug.com/519664497.
} // namespace internalFood for thought: IIRC we also had issues where the main thread needed to block "some time". Can we have a similar helper that for `BlockForAt(timeout, synchronziation point)` that schedules a background task which unblock the point before it goes to sleep?
If you mean the same primitive as in the CL but that "sleeps" for a given timeout instead of waiting indefinitely for "resume" - yes, that was the plan for a follow-up CL; the same groundwork would work.
If you mean allowing e.g. a Web Worker to block/resume operations on the main thread, then it might be possible as well but I haven't thought it through very well (at least having the state in `IsolateGroup` should allow for that).
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 5 comments![]( "Open in Gerrit")
defines += [ "V8_ENABLE_SYNCHRONIZATION_POINT_API" ]I wonder if we need this with my comment below. Would be nice to just be able to use it on any build with --allow-natives-syntax
std::string_view synchronization_point) {I think you may not need a separate compile mode if you create a guard here.
IsolateGroup:Current() for Chrome is fast because we only have a single one. We can guard this with a single relaxed load here.
synchronization_point_data_for_testing_.find(synchronization_point);This always performs lock + lookup, even when not used. The bailout will fix this as well.
std::pair<MaybeHandle<Code>, BailoutReason> MaglevCompiler::GenerateCode(I think this is main thread. How would this work with the current approach?
} // namespace internalMaksim IvanovFood for thought: IIRC we also had issues where the main thread needed to block "some time". Can we have a similar helper that for `BlockForAt(timeout, synchronziation point)` that schedules a background task which unblock the point before it goes to sleep?
If you mean the same primitive as in the CL but that "sleeps" for a given timeout instead of waiting indefinitely for "resume" - yes, that was the plan for a follow-up CL; the same groundwork would work.
If you mean allowing e.g. a Web Worker to block/resume operations on the main thread, then it might be possible as well but I haven't thought it through very well (at least having the state in `IsolateGroup` should allow for that).
If you mean the same primitive as in the CL but that "sleeps" for a given timeout instead of waiting indefinitely for "resume" - yes, that was the plan for a follow-up CL; the same groundwork would work.
This is what I meant yeah. It should unblock waiting on the main thread that is triggered from the main thread.
If you mean allowing e.g. a Web Worker to block/resume operations on the main thread, then it might be possible as well but I haven't thought it through very well (at least having the state in `IsolateGroup` should allow for that).
IIUC this should already work based on the current CL here. It's a bit annoyin to set up though.
Related details
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Maksim Ivanov
| Commit-Queue | +1 |
3 comments
+arashk@ for another pair of eyes, and also because of a verifier test issue.
std::pair<MaybeHandle<Code>, BailoutReason> MaglevCompiler::GenerateCode(I think this is main thread. How would this work with the current approach?
I've updated the CL - I believe now the main-thread stuff can be blocked from e.g. Web Workers.
let id = 615; // Runtime::kWasmAllocateFeedbackVectorThis is unfortunate but the test hardcodes a specific ID, and every CL that shifts numbers will break it. I'll think about a way to address it in a follow-up (a dictionary of builtin names?).
Related details
- Arash Kazemi
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 3 comments![]( "Open in Gerrit")
defines += [ "V8_ENABLE_SYNCHRONIZATION_POINT_API" ]I wonder if we need this with my comment below. Would be nice to just be able to use it on any build with --allow-natives-syntax
Done
I think you may not need a separate compile mode if you create a guard here.
IsolateGroup:Current() for Chrome is fast because we only have a single one. We can guard this with a single relaxed load here.
Done - indeed this seems to be neutral on benchmarks! Thanks.
synchronization_point_data_for_testing_.find(synchronization_point);This always performs lock + lookup, even when not used. The bailout will fix this as well.
Done. We could also add a cheaper bailout via an atomic flag, but in my measurement this only slows down --allow-native-syntax tests by 1%, so probably not necessary?
Maksim Ivanov (Gerrit)
Maksim Ivanov added 1 comment![]( "Open in Gerrit")
RUNTIME_FUNCTION(Runtime_BlockAt) {@mliedtke: Does this API look good from the Fuzzilli perspective? I presume I'll need to write a custom generator with the hardcoded list of string arguments in Fuzzilli.
Or would it be much better if V8 maintained a list of all synchronization point names, so that Fuzzilli could fetch it? (another function call, a global constant, etc.)
Related details
- Arash Kazemi
- Matthias Liedtke
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 1 comment![]( "Open in Gerrit")
RUNTIME_FUNCTION(Runtime_BlockAt) {@mliedtke: Does this API look good from the Fuzzilli perspective? I presume I'll need to write a custom generator with the hardcoded list of string arguments in Fuzzilli.
Or would it be much better if V8 maintained a list of all synchronization point names, so that Fuzzilli could fetch it? (another function call, a global constant, etc.)
And another question is whether to cap the sleep at e.g. N seconds, to prevent complete deadlocks.
Matthias Liedtke (Gerrit)
Matthias Liedtke added 1 comment![]( "Open in Gerrit")
RUNTIME_FUNCTION(Runtime_BlockAt) {@mliedtke: Does this API look good from the Fuzzilli perspective? I presume I'll need to write a custom generator with the hardcoded list of string arguments in Fuzzilli.
Or would it be much better if V8 maintained a list of all synchronization point names, so that Fuzzilli could fetch it? (another function call, a global constant, etc.)
How would that work for a fuzzer? How would a fuzzer make use of it?
Realistically it would probably only ever do the `%BlockAt` reliably and then we'd wait until timeout (1-2 seconds) and then just restart the d8 executable.
`%SetDelayAt("foo", 100 /*ms*/);` or something similar could be used by a fuzzer but even then, do we expect that a fuzzer will stumble upon useful invocations of this?
It won't know whether it needs to trigger a tier-up or a gc or whichever action that is necessary to reach the "foo" point and if it isn't a delay but actually a block as proposed here, it would need multi-threading if this would cause the block to happen on the main thread (e.g. something that isn't background-compilation or background-gc). If it happens on the background, it would still need to actively delay the time before it calls `%Resume()` as JS doesn't have a sleep function (and `setTimeout` doesn't actually wait until the timeout is reached in d8 I think).
Related details
- Arash Kazemi
- Maksim Ivanov
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arash Kazemi (Gerrit)
Arash Kazemi added 1 comment![]( "Open in Gerrit")
let id = 615; // Runtime::kWasmAllocateFeedbackVectorThis is unfortunate but the test hardcodes a specific ID, and every CL that shifts numbers will break it. I'll think about a way to address it in a follow-up (a dictionary of builtin names?).
I think we can move it to `bytecode-verifier-unittest`, from what I can see there is no requirement for this to be a mjsunit test. We already have a similar test there (`ForbiddenRuntimeFunction`) so maybe we don't even care that much about keeping this specific function in the regressions or we can parameterize the test.
Related details
- Maksim Ivanov
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
RUNTIME_FUNCTION(Runtime_BlockAt) {Matthias Liedtke@mliedtke: Does this API look good from the Fuzzilli perspective? I presume I'll need to write a custom generator with the hardcoded list of string arguments in Fuzzilli.
Or would it be much better if V8 maintained a list of all synchronization point names, so that Fuzzilli could fetch it? (another function call, a global constant, etc.)
How would that work for a fuzzer? How would a fuzzer make use of it?
Realistically it would probably only ever do the `%BlockAt` reliably and then we'd wait until timeout (1-2 seconds) and then just restart the d8 executable.
`%SetDelayAt("foo", 100 /*ms*/);` or something similar could be used by a fuzzer but even then, do we expect that a fuzzer will stumble upon useful invocations of this?It won't know whether it needs to trigger a tier-up or a gc or whichever action that is necessary to reach the "foo" point and if it isn't a delay but actually a block as proposed here, it would need multi-threading if this would cause the block to happen on the main thread (e.g. something that isn't background-compilation or background-gc). If it happens on the background, it would still need to actively delay the time before it calls `%Resume()` as JS doesn't have a sleep function (and `setTimeout` doesn't actually wait until the timeout is reached in d8 I think).
`%SetDelayAt()` is also what I was suggesting below. Maybe that can be a follow up?
Related details
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 4 comments![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
4 comments
lgtm, I think we can iterate on this
void SetBlockAtSynchronizationPointForTesting(super-nit: Can we move the `*ForTesting()` methods to the end of the public section? (This is not strictly enforced anywhere but it is often nice to navigate this way.)
if (V8_LIKELY(!v8_flags.allow_natives_syntax)) return;Maybe another bool like `ever_set_any_synchronization_point_` would be better? (To keep it independent of --allow-natives-syntax)
if (V8_LIKELY(!v8_flags.allow_natives_syntax)) return;nit: new code often uses [[likely]] already (which is supported)
Related details
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
- Leszek Swirski
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 8 comments![]( "Open in Gerrit")
super-nit: Can we move the `*ForTesting()` methods to the end of the public section? (This is not strictly enforced anywhere but it is often nice to navigate this way.)
Done
if (V8_LIKELY(!v8_flags.allow_natives_syntax)) return;Maybe another bool like `ever_set_any_synchronization_point_` would be better? (To keep it independent of --allow-natives-syntax)
Done (called it "any_synchronization_point_for_testing_" for consistency with other symbols).
if (V8_LIKELY(!v8_flags.allow_natives_syntax)) return;nit: new code often uses [[likely]] already (which is supported)
Done
std::pair<MaybeHandle<Code>, BailoutReason> MaglevCompiler::GenerateCode(I think this is main thread. How would this work with the current approach?
I've updated the CL - I believe now the main-thread stuff can be blocked from e.g. Web Workers.
resolving
RUNTIME_FUNCTION(Runtime_BlockAt) {Matthias Liedtke@mliedtke: Does this API look good from the Fuzzilli perspective? I presume I'll need to write a custom generator with the hardcoded list of string arguments in Fuzzilli.
Or would it be much better if V8 maintained a list of all synchronization point names, so that Fuzzilli could fetch it? (another function call, a global constant, etc.)
Michael LippautzHow would that work for a fuzzer? How would a fuzzer make use of it?
Realistically it would probably only ever do the `%BlockAt` reliably and then we'd wait until timeout (1-2 seconds) and then just restart the d8 executable.
`%SetDelayAt("foo", 100 /*ms*/);` or something similar could be used by a fuzzer but even then, do we expect that a fuzzer will stumble upon useful invocations of this?It won't know whether it needs to trigger a tier-up or a gc or whichever action that is necessary to reach the "foo" point and if it isn't a delay but actually a block as proposed here, it would need multi-threading if this would cause the block to happen on the main thread (e.g. something that isn't background-compilation or background-gc). If it happens on the background, it would still need to actively delay the time before it calls `%Resume()` as JS doesn't have a sleep function (and `setTimeout` doesn't actually wait until the timeout is reached in d8 I think).
`%SetDelayAt()` is also what I was suggesting below. Maybe that can be a follow up?
`SetDelayAt()` would be one way, yes - planned for a follow-up.
Another pattern, possible immediately with this CL, would be (also shown in the `regress-502997649.js` from the latest patchset):
```
// main thread:
%BlockAt(xyz);
... do something to trigger a bg job hitting `xyz` ...
... optionally, a busy-loop sleep ...
%Resume(xyz);
```
This second pattern can also be made more robust if we provide `%WaitUntilBlocked()` - to avoid the need in busy-loop waits.
P.S. Generally, yes, it's clear that the fuzzer won't intrinsically know how a particular `xyz` string arg is correlated with interesting candidates for the `do something` block. Still I think there's a chance it can discover something interesting when randomly mutating according to this pattern - but it needs to pass a really existing argument to `BlockAt`/`Resume`, not some random garbage that'd be ignored.
As a showcase for the API, crafted a POC for an older crbug.com/502997649. This snippet triggers sandbox violation detection when reverting the fix for that bug.
let id = 615; // Runtime::kWasmAllocateFeedbackVectorArash KazemiThis is unfortunate but the test hardcodes a specific ID, and every CL that shifts numbers will break it. I'll think about a way to address it in a follow-up (a dictionary of builtin names?).
I think we can move it to `bytecode-verifier-unittest`, from what I can see there is no requirement for this to be a mjsunit test. We already have a similar test there (`ForbiddenRuntimeFunction`) so maybe we don't even care that much about keeping this specific function in the regressions or we can parameterize the test.
- Arash Kazemi
- Leszek Swirski
- Matthias Liedtke
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arash Kazemi (Gerrit)
Arash Kazemi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Arash Kazemi
| Code-Review | +1 |
2 comments
LGTM thanks!
IsolateGroup::current()->DoSynchronizationPointForTesting(sync_point_name)Although I don't think it should, did you measure how much checking for synchronization points impacts compilation times for example for maglev?
Related details
- Leszek Swirski
- Maksim Ivanov
- Matthias Liedtke
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 2 comments![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Code-Review | +1 |
2 comments
LGTM on the design
RUNTIME_FUNCTION(Runtime_BlockAt) {Matthias Liedtke@mliedtke: Does this API look good from the Fuzzilli perspective? I presume I'll need to write a custom generator with the hardcoded list of string arguments in Fuzzilli.
Or would it be much better if V8 maintained a list of all synchronization point names, so that Fuzzilli could fetch it? (another function call, a global constant, etc.)
Michael LippautzHow would that work for a fuzzer? How would a fuzzer make use of it?
Realistically it would probably only ever do the `%BlockAt` reliably and then we'd wait until timeout (1-2 seconds) and then just restart the d8 executable.
`%SetDelayAt("foo", 100 /*ms*/);` or something similar could be used by a fuzzer but even then, do we expect that a fuzzer will stumble upon useful invocations of this?It won't know whether it needs to trigger a tier-up or a gc or whichever action that is necessary to reach the "foo" point and if it isn't a delay but actually a block as proposed here, it would need multi-threading if this would cause the block to happen on the main thread (e.g. something that isn't background-compilation or background-gc). If it happens on the background, it would still need to actively delay the time before it calls `%Resume()` as JS doesn't have a sleep function (and `setTimeout` doesn't actually wait until the timeout is reached in d8 I think).
Maksim Ivanov`%SetDelayAt()` is also what I was suggesting below. Maybe that can be a follow up?
`SetDelayAt()` would be one way, yes - planned for a follow-up.
Another pattern, possible immediately with this CL, would be (also shown in the `regress-502997649.js` from the latest patchset):
```
// main thread:
%BlockAt(xyz);
... do something to trigger a bg job hitting `xyz` ...
... optionally, a busy-loop sleep ...
%Resume(xyz);
```This second pattern can also be made more robust if we provide `%WaitUntilBlocked()` - to avoid the need in busy-loop waits.
P.S. Generally, yes, it's clear that the fuzzer won't intrinsically know how a particular `xyz` string arg is correlated with interesting candidates for the `do something` block. Still I think there's a chance it can discover something interesting when randomly mutating according to this pattern - but it needs to pass a really existing argument to `BlockAt`/`Resume`, not some random garbage that'd be ignored.
Making it pass something predefined is easy, that can be done by a generator.
I agree that it can just by accident generate patterns where it emits `BlockAt(xyz);` and then something that might hit xyz.
The problem is that without the `%Resume(xyz)` it's going to timeout and not be added to the corpus. So this isn't something that the fuzzer can incrementally explore by adding one more thing each step but instead it needs to basically generate it in one attempt which is unlikely unless you write a generator that emits all the missing pieces in one go which might be hard. And then blocking and resuming itself isn't actually testing anything interesting, unless you make it also do something that potentially has side effects between blocking and resuming so that this test case is actually interesting. Having existing test cases with these patterns in the regression tests that we import would certainly help there as well.
The API itself sounds fine, feel free to go ahead with it, just saying that the fuzzer won't explore this in the same way as it can explore complex JS test cases due to the mentioned issues.
Related details
- Leszek Swirski
- Maksim Ivanov
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 2 comments![]( "Open in Gerrit")
IsolateGroup::current()->DoSynchronizationPointForTesting(sync_point_name)Although I don't think it should, did you measure how much checking for synchronization points impacts compilation times for example for maglev?
I tried benchmarking via `BenchMaglev` and haven't seen any measurable difference. In general, I've seen some heavy mjsunit tests running ~1% slower with this change.
If it becomes too bad in the future (e.g. if we add many SYNCHRONIZATION_POINTs to hot paths) we can consider further optimizations and/or an `ifdef`.
RUNTIME_FUNCTION(Runtime_BlockAt) {Matthias Liedtke@mliedtke: Does this API look good from the Fuzzilli perspective? I presume I'll need to write a custom generator with the hardcoded list of string arguments in Fuzzilli.
Or would it be much better if V8 maintained a list of all synchronization point names, so that Fuzzilli could fetch it? (another function call, a global constant, etc.)
Michael LippautzHow would that work for a fuzzer? How would a fuzzer make use of it?
Realistically it would probably only ever do the `%BlockAt` reliably and then we'd wait until timeout (1-2 seconds) and then just restart the d8 executable.
`%SetDelayAt("foo", 100 /*ms*/);` or something similar could be used by a fuzzer but even then, do we expect that a fuzzer will stumble upon useful invocations of this?It won't know whether it needs to trigger a tier-up or a gc or whichever action that is necessary to reach the "foo" point and if it isn't a delay but actually a block as proposed here, it would need multi-threading if this would cause the block to happen on the main thread (e.g. something that isn't background-compilation or background-gc). If it happens on the background, it would still need to actively delay the time before it calls `%Resume()` as JS doesn't have a sleep function (and `setTimeout` doesn't actually wait until the timeout is reached in d8 I think).
Maksim Ivanov`%SetDelayAt()` is also what I was suggesting below. Maybe that can be a follow up?
Matthias Liedtke`SetDelayAt()` would be one way, yes - planned for a follow-up.
Another pattern, possible immediately with this CL, would be (also shown in the `regress-502997649.js` from the latest patchset):
```
// main thread:
%BlockAt(xyz);
... do something to trigger a bg job hitting `xyz` ...
... optionally, a busy-loop sleep ...
%Resume(xyz);
```This second pattern can also be made more robust if we provide `%WaitUntilBlocked()` - to avoid the need in busy-loop waits.
P.S. Generally, yes, it's clear that the fuzzer won't intrinsically know how a particular `xyz` string arg is correlated with interesting candidates for the `do something` block. Still I think there's a chance it can discover something interesting when randomly mutating according to this pattern - but it needs to pass a really existing argument to `BlockAt`/`Resume`, not some random garbage that'd be ignored.
Making it pass something predefined is easy, that can be done by a generator.
I agree that it can just by accident generate patterns where it emits `BlockAt(xyz);` and then something that might hit xyz.
The problem is that without the `%Resume(xyz)` it's going to timeout and not be added to the corpus. So this isn't something that the fuzzer can incrementally explore by adding one more thing each step but instead it needs to basically generate it in one attempt which is unlikely unless you write a generator that emits all the missing pieces in one go which might be hard. And then blocking and resuming itself isn't actually testing anything interesting, unless you make it also do something that potentially has side effects between blocking and resuming so that this test case is actually interesting. Having existing test cases with these patterns in the regression tests that we import would certainly help there as well.The API itself sounds fine, feel free to go ahead with it, just saying that the fuzzer won't explore this in the same way as it can explore complex JS test cases due to the mentioned issues.
- Arash Kazemi
- Leszek Swirski
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arash Kazemi (Gerrit)
Arash Kazemi added 1 comment![]( "Open in Gerrit")
IsolateGroup::current()->DoSynchronizationPointForTesting(sync_point_name)Maksim IvanovAlthough I don't think it should, did you measure how much checking for synchronization points impacts compilation times for example for maglev?
I tried benchmarking via `BenchMaglev` and haven't seen any measurable difference. In general, I've seen some heavy mjsunit tests running ~1% slower with this change.
If it becomes too bad in the future (e.g. if we add many SYNCHRONIZATION_POINTs to hot paths) we can consider further optimizations and/or an `ifdef`.
- Leszek Swirski
- Maksim Ivanov
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Leszek Swirski
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 3 comments![]( "Open in Gerrit")
isolate->isolate_group()->SetBlockAtSynchronizationPointForTesting(can we also register the thread Id that requested the block, and CHECK that it's not the same as the thread that is about to pause? To detect deadlock in another way than timeouts
%OptimizeFunctionOnNextCall(g, 'concurrent');I would expect this to be paired with `%WaitForBackgroundOptimization()`, probably instead of the second busy loop.
let start = Date.now();
while (Date.now() - start < 1000) {}instead of busy looping here, should there be a `%WaitUntilBlocked('TurbofanEarlyGraphTrimming')`?
Related details
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 4 comments![]( "Open in Gerrit")
isolate->isolate_group()->SetBlockAtSynchronizationPointForTesting(can we also register the thread Id that requested the block, and CHECK that it's not the same as the thread that is about to pause? To detect deadlock in another way than timeouts
Done
As a showcase for the API, crafted a POC for an older crbug.com/502997649. This snippet triggers sandbox violation detection when reverting the fix for that bug.
resolving.
I would expect this to be paired with `%WaitForBackgroundOptimization()`, probably instead of the second busy loop.
Done
let start = Date.now();
while (Date.now() - start < 1000) {}instead of busy looping here, should there be a `%WaitUntilBlocked('TurbofanEarlyGraphTrimming')`?
Acknowledged - Yes, will add this in a follow-up. Just focusing on the first CL with the basic functionality for now.
Related details
- Arash Kazemi
- Leszek Swirski
- Matthias Liedtke
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted Code-Review+1![]( "Open in Gerrit")
- Arash Kazemi
- Leszek Swirski
- Maksim Ivanov
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Arash Kazemi
- Maksim Ivanov
- Matthias Liedtke
Maksim Ivanov (Gerrit)
Maksim Ivanov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Maksim Ivanov
| Commit-Queue | +2 |
3 comments
[testing] Block/resume primitiveI've checked that this API works to craft a straightforward POC for crbug.com/514446876. And that adding one more `SYNCHRONIZATION_POINT_FOR_TESTING` gives a reliable POC for crbug.com/519664497.
resolving
} // namespace internalMaksim IvanovFood for thought: IIRC we also had issues where the main thread needed to block "some time". Can we have a similar helper that for `BlockForAt(timeout, synchronziation point)` that schedules a background task which unblock the point before it goes to sleep?
Michael LippautzIf you mean the same primitive as in the CL but that "sleeps" for a given timeout instead of waiting indefinitely for "resume" - yes, that was the plan for a follow-up CL; the same groundwork would work.
If you mean allowing e.g. a Web Worker to block/resume operations on the main thread, then it might be possible as well but I haven't thought it through very well (at least having the state in `IsolateGroup` should allow for that).
If you mean the same primitive as in the CL but that "sleeps" for a given timeout instead of waiting indefinitely for "resume" - yes, that was the plan for a follow-up CL; the same groundwork would work.
This is what I meant yeah. It should unblock waiting on the main thread that is triggered from the main thread.
If you mean allowing e.g. a Web Worker to block/resume operations on the main thread, then it might be possible as well but I haven't thought it through very well (at least having the state in `IsolateGroup` should allow for that).
IIUC this should already work based on the current CL here. It's a bit annoyin to set up though.
- Arash Kazemi
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Maksim Ivanov
| Commit-Queue | +2 |
Still not fully polished, but sharing for initial feedback. Using a GN flag guard to avoid having to think about performance in cross-thread syncing logic.
resolving
Related details
- Arash Kazemi
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
v8-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
v8-s...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
[testing] Block/resume primitive
Allow fuzzers and tests to block&resume at predefined strategic
locations in V8 - "synchronization points". This should help fuzzers and
developers create deterministic reproducers for specific data races.
The C++ code has to be prepared via the
`SYNCHRONIZATION_POINT_FOR_TESTING("foo")` macros. The JS code can then
call `%BlockAt("foo")` and `%Resume("foo")` to halt/unhalt the code
reaching the corresponding location (when the natives are enabled).
- M src/compiler/phase.h
- M src/compiler/pipeline.cc
- M src/compiler/turboshaft/phase.h
- M src/compiler/turboshaft/pipelines.h
- M src/compiler/turboshaft/turbolev-frontend-pipeline.cc
- M src/handles/persistent-handles.cc
- M src/init/isolate-group.cc
- M src/init/isolate-group.h
- M src/maglev/maglev-compiler.cc
- M src/maglev/maglev-concurrent-dispatcher.cc
- M src/runtime/runtime-test.cc
- M src/runtime/runtime.h
- A test/mjsunit/sandbox/regress/regress-502997649.js
Code-Review: +1 by Leszek Swirski, +1 by Michael Lippautz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |