[canvas] Add a feature to defer hibernation [chromium/src : main]
Benoit Lize (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Jean-Philippe Gravel
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 18 comments![]( "Open in Gerrit")
- Wasteful: In case of rapid back and forthAlso note that according to the `Blink.Canvas.HibernationEvents` histogram in UMA, 82% of hibernations abort because the tab does background rendering. This is definitely harmful because in these cases, we hibernated for no reason. This CL provides the mechanism to address this!
CanvasHibernationHandler::HibernationEvent::kHibernationScheduled, 1);This is unfortunate. This test used to give us confidence that two visibility cycle in a row would only trigger hibernation once. How do we know that the code is well behaved now?
Could we add a new `HibernationEvent`, increased when hibernation is actually done? You could check that it has a count of `1` in this test, to make sure that hibernation only happened once despite being scheduled twice. It would also be useful to monitor the feature once launched, because otherwise, you'd have no (direct) way of knowing the number of time delayed hibernation was triggered.
I could similarly be useful to have an event incremented when the canvas is compressed, so we'd have visibility on the hit rate of that code path.
I think that there's something missing here. Originally, we called `HibernateOrLogFailure` on an idle task and this task ended up setting `image_`. From this point, `IsHibernating()` would return true. `IsHibernating` is what we check to know we must wake up from hibernation [if the page does background rendering](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc;l=1457-1485;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18), of if [the page becomes visible again](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc;l=1040-1042;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18). Because `HibernateOrLogFailure` was called as soon as idle (hopefully before the next JavaScript task, but maybe not?), `IsHibernating()` returned mostly the right value.
With the new code though, `IsHibernating()` will only start to return `true` after several minutes. This leaves plenty of time of the page to restart rendering in the background (82% of hibernation cases). If this happens, `Hibernate()` will run unimpeded because the page is still hidden. But that will hibernate the canvas while it's being used in the background.
DCHECK(handlers_.Contains(handler));What changed in this CL for `Unregister` to be called when `Register` wasn't?
// The canvas was hibernated again since the task was posted, let the next
// task deal with it.The comment describes the `else` case which isn't even there. Can you update the comment to describe the positive case instead? Maybe just prefixing with `If` would be enough: `If the canvas was hibernated ...`.
if (hibernation_scheduled_) {Have you verified that `InitiateHibernationIfNecessary` is never called multiple times for a given canvas and page visibility change event?
epoch_++;Nit: do `++epoch_` instead ([required by the style guide](https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement)).
auto params = GetParam();Nit: avoid using `auto` โmerely to avoid the inconvenience of writing an explicit type.โ
https://google.github.io/styleguide/cppguide.html#Type_deduction
Same for the other new usages of `auto` below (except for the `base::MakeRefCounted` factory method because the type is already spelled explicitly as template parameter, that's one of the few cases where `auto` is recommended by https://abseil.io/tips/232).
Here however, the variable `params` provides very little value. You could very well just remove it and inline `GetParam()` below.
::testing::Combine(Nit, add using declarations for `testing::` symbols, to improve clarity. This is recommended by internal guidelines, but also by the official GoogleTest documentation:
โNote: gMock lives in the `testing` name space. For readability, it is recommended to write `using ::testing::Foo;` once in your file before using the name `Foo` defined by gMock. We omit such `using` statements in this section for brevity, but you should do it in your own code.โ
https://google.github.io/googletest/gmock_cook_book.html
The idea is that GoogleTest statements are designed to roughly read like english sentenses, e.g. `EXPECT_THAT(foo, SizeIs(Lt(3)))` -> "expect that foo's size is less than 3". Adding namespace qualification everywhere hampers the clarity of these statements.
if (!base::FeatureList::IsEnabled(features::kCanvas2DHibernationDefer)) {
GTEST_SKIP();
}Why skipping this test? The original test seemed useful for the normal operation mode. Why drop the test coverage?
TEST_P(CanvasHibernationHandlerTest, ForegroundBeforeHibernationStarts) {This test looks identical to `ForegroundTooEarly` above. Or am I missing something?.
TEST_P(CanvasHibernationHandlerTest, ForegroundBeforeHibernationStarts) {Shouldn't this be something like "ForegroundBeforeEncodingStarts"? Hibernation did happen, `IsHibernating` is true.
auto task_runner = base::MakeRefCounted<TestSingleThreadTaskRunner>();Shouldn't the test do `handler.SetBackgroundTaskRunnerForTesting(task_runner);`? If this works without, why are all other tests doing it? Could you remove that from other tests?
TEST_P(CanvasHibernationHandlerTest, ForegroundAfterEncoding) {The new test version seems to foregrounds **before** encoding, not after. Isn't it essentially testing the same thing as `ForegroundTooEarly` and the new test above? Could you keep logic waiting for the encoding?
TEST_P(CanvasHibernationHandlerTest, ForegroundFlipForAfterEncoding) {Was this meaning to say "FlipFlop"?
// EXPECT_TRUE(handler.IsHibernating());Why the commented code? Delete or restore?
delay = WaitForHibernation();There's already a `WaitForHibernation()` on line 432. Isn't this one redundant?
TEST_P(CanvasHibernationHandlerTest, ForegroundFlipForBeforeEncoding) {Same here (`FlipFlop`)?
Related details
- Benoit Lize
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 3 comments![]( "Open in Gerrit")
BASE_FEATURE(kCanvas2DHibernationDefer,Please keep features sorted by identifier name. `kCanvas2DHibernationDefer` should be placed after `kCanvas2DHibernation` to maintain alphabetical order. (Blink Style Guide: Feature definitions and associated constants)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your ๐ feedback ๐ to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kCanvas2DHibernationDefer);Please keep feature declarations sorted by identifier name. `kCanvas2DHibernationDefer` should be placed after `kCanvas2DHibernation` to maintain alphabetical order. (Blink Style Guide: Feature declarations and associated constants)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your ๐ feedback ๐ to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
void SaveForHibernation(sk_sp<SkImage>&& image,nit: Per the style guide, obvious parameter names may be omitted from function declarations in header files. Consider removing the names for `image`, `recorder`, and `context` as their purpose is clear from their types. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your ๐ feedback ๐ to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- Benoit Lize
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benoit Lize (Gerrit)
Benoit Lize added 17 comments![]( "Open in Gerrit")
CanvasHibernationHandler::HibernationEvent::kHibernationScheduled, 1);This is unfortunate. This test used to give us confidence that two visibility cycle in a row would only trigger hibernation once. How do we know that the code is well behaved now?
Could we add a new `HibernationEvent`, increased when hibernation is actually done? You could check that it has a count of `1` in this test, to make sure that hibernation only happened once despite being scheduled twice. It would also be useful to monitor the feature once launched, because otherwise, you'd have no (direct) way of knowing the number of time delayed hibernation was triggered.
I could similarly be useful to have an event incremented when the canvas is compressed, so we'd have visibility on the hit rate of that code path.
The foreground / background cycles are tested in CanvasHibernationHandler tests, which are admittedly lower level.
Added an element to the enum, and also fixed the enum, which was out of sync. Did not change the name of the histogram or enum, since it was the XML that was incorrect, not the emitting code that changed, and it's only adding a new value to the enum.
I think that there's something missing here. Originally, we called `HibernateOrLogFailure` on an idle task and this task ended up setting `image_`. From this point, `IsHibernating()` would return true. `IsHibernating` is what we check to know we must wake up from hibernation [if the page does background rendering](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc;l=1457-1485;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18), of if [the page becomes visible again](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc;l=1040-1042;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18). Because `HibernateOrLogFailure` was called as soon as idle (hopefully before the next JavaScript task, but maybe not?), `IsHibernating()` returned mostly the right value.
With the new code though, `IsHibernating()` will only start to return `true` after several minutes. This leaves plenty of time of the page to restart rendering in the background (82% of hibernation cases). If this happens, `Hibernate()` will run unimpeded because the page is still hidden. But that will hibernate the canvas while it's being used in the background.
I guess there are a couple solutions here: we could either give up on hibernation when rendering happens in background, or only hibernate if the canvas hasn't been rendering for a while.
The first one is more conservative and easier to implement, by bumping `epoch_` in `Clear()`. This is what I've done in this CL. wdyt?
DCHECK(handlers_.Contains(handler));What changed in this CL for `Unregister` to be called when `Register` wasn't?
Added a comment in the code.
// The canvas was hibernated again since the task was posted, let the next
// task deal with it.The comment describes the `else` case which isn't even there. Can you update the comment to describe the positive case instead? Maybe just prefixing with `If` would be enough: `If the canvas was hibernated ...`.
Done
if (hibernation_scheduled_) {Have you verified that `InitiateHibernationIfNecessary` is never called multiple times for a given canvas and page visibility change event?
The only place that calls this is CanvasRenderingContext2D::PageVisibilityChanged(), which is called once.
And even if called several times, it is only a minor inefficiency, since it just posts a task that will do nothing.
Nit: do `++epoch_` instead ([required by the style guide](https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement)).
Done
Nit: avoid using `auto` โmerely to avoid the inconvenience of writing an explicit type.โ
https://google.github.io/styleguide/cppguide.html#Type_deductionSame for the other new usages of `auto` below (except for the `base::MakeRefCounted` factory method because the type is already spelled explicitly as template parameter, that's one of the few cases where `auto` is recommended by https://abseil.io/tips/232).
Here however, the variable `params` provides very little value. You could very well just remove it and inline `GetParam()` below.
Done
Nit, add using declarations for `testing::` symbols, to improve clarity. This is recommended by internal guidelines, but also by the official GoogleTest documentation:
โNote: gMock lives in the `testing` name space. For readability, it is recommended to write `using ::testing::Foo;` once in your file before using the name `Foo` defined by gMock. We omit such `using` statements in this section for brevity, but you should do it in your own code.โ
https://google.github.io/googletest/gmock_cook_book.htmlThe idea is that GoogleTest statements are designed to roughly read like english sentenses, e.g. `EXPECT_THAT(foo, SizeIs(Lt(3)))` -> "expect that foo's size is less than 3". Adding namespace qualification everywhere hampers the clarity of these statements.
Done
if (!base::FeatureList::IsEnabled(features::kCanvas2DHibernationDefer)) {
GTEST_SKIP();
}Why skipping this test? The original test seemed useful for the normal operation mode. Why drop the test coverage?
Done.
TEST_P(CanvasHibernationHandlerTest, ForegroundBeforeHibernationStarts) {Shouldn't this be something like "ForegroundBeforeEncodingStarts"? Hibernation did happen, `IsHibernating` is true.
Done
TEST_P(CanvasHibernationHandlerTest, ForegroundBeforeHibernationStarts) {This test looks identical to `ForegroundTooEarly` above. Or am I missing something?.
Done
auto task_runner = base::MakeRefCounted<TestSingleThreadTaskRunner>();Shouldn't the test do `handler.SetBackgroundTaskRunnerForTesting(task_runner);`? If this works without, why are all other tests doing it? Could you remove that from other tests?
Done
TEST_P(CanvasHibernationHandlerTest, ForegroundAfterEncoding) {The new test version seems to foregrounds **before** encoding, not after. Isn't it essentially testing the same thing as `ForegroundTooEarly` and the new test above? Could you keep logic waiting for the encoding?
Changed the tests, to avoid redundant, and keep the same / better coverage. Thanks for the comment.
TEST_P(CanvasHibernationHandlerTest, ForegroundFlipForAfterEncoding) {Was this meaning to say "FlipFlop"?
Done
Why the commented code? Delete or restore?
Done
TEST_P(CanvasHibernationHandlerTest, ForegroundFlipForBeforeEncoding) {Same here (`FlipFlop`)?
- Jean-Philippe Gravel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 15 comments![]( "Open in Gerrit")
CanvasHibernationHandler::HibernationEvent::kHibernationScheduled, 1);Benoit LizeThis is unfortunate. This test used to give us confidence that two visibility cycle in a row would only trigger hibernation once. How do we know that the code is well behaved now?
Could we add a new `HibernationEvent`, increased when hibernation is actually done? You could check that it has a count of `1` in this test, to make sure that hibernation only happened once despite being scheduled twice. It would also be useful to monitor the feature once launched, because otherwise, you'd have no (direct) way of knowing the number of time delayed hibernation was triggered.
I could similarly be useful to have an event incremented when the canvas is compressed, so we'd have visibility on the hit rate of that code path.
The foreground / background cycles are tested in CanvasHibernationHandler tests, which are admittedly lower level.
Added an element to the enum, and also fixed the enum, which was out of sync. Did not change the name of the histogram or enum, since it was the XML that was incorrect, not the emitting code that changed, and it's only adding a new value to the enum.
Added an element to the enum
Sounds good.
and also fixed the enum, which was out of sync
Good catch! Thanks for fixing this.
void SaveForHibernation(sk_sp<SkImage>&& image,nit: Per the style guide, obvious parameter names may be omitted from function declarations in header files. Consider removing the names for `image`, `recorder`, and `context` as their purpose is clear from their types. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your ๐ feedback ๐ to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
The style guide says "may omit", which I take as an optional recommendation. FWIW, I'm not a fan of this guideline because it prevents clang-tidy from validating `/*parameter_name=*/` comments at call sites. Here, `recorder` is a pointer, so what if a caller ever wanted to do `/*recorder=*/nullptr`? That doesn't really make sense in this particular example, but it goes to show that this general guideline is flawed.
I wonder if this recommendation is still up to date. Maybe it predates clang-tidy's check?
In any case, don't mind me. Feel free to follow the recommendation if you want. It's in the style guide! ;)
kMaxValue = kHibernationAbortedBecauseNoSurface,Update kMaxValue.
Benoit LizeI think that there's something missing here. Originally, we called `HibernateOrLogFailure` on an idle task and this task ended up setting `image_`. From this point, `IsHibernating()` would return true. `IsHibernating` is what we check to know we must wake up from hibernation [if the page does background rendering](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc;l=1457-1485;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18), of if [the page becomes visible again](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc;l=1040-1042;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18). Because `HibernateOrLogFailure` was called as soon as idle (hopefully before the next JavaScript task, but maybe not?), `IsHibernating()` returned mostly the right value.
With the new code though, `IsHibernating()` will only start to return `true` after several minutes. This leaves plenty of time of the page to restart rendering in the background (82% of hibernation cases). If this happens, `Hibernate()` will run unimpeded because the page is still hidden. But that will hibernate the canvas while it's being used in the background.
I guess there are a couple solutions here: we could either give up on hibernation when rendering happens in background, or only hibernate if the canvas hasn't been rendering for a while.
The first one is more conservative and easier to implement, by bumping `epoch_` in `Clear()`. This is what I've done in this CL. wdyt?
Increasing `epoch_` sounds reasonable. But I wonder what will happen if you toggle between tabs a lot. Say you have many tabs each with lots of canvases and you ctrl-tab constantly for 5 minutes. How many pending tasks will accumulate? Can this be a problem? Is there a way of actively cancelling tasks?
DCHECK(handlers_.Contains(handler));Benoit LizeWhat changed in this CL for `Unregister` to be called when `Register` wasn't?
Added a comment in the code.
You mean this comment?
```
// Hibernation may not be pending, we may not be registered.
```
Comments should explains the why, but not the what. I still understand how it's possible for `Clear()` to be called if `Register()` wasn't. All calls to `Clear()` are guarded by `if (IsHibernating())` checks and `IsHibernating()` is only `true` if `Register()` is called. Isn't it?
That being said, I suppose we are better off without the `DCHECK` regardless... It's not protecting against anything harmful. `handlers_.erase(handler);` is a no-op if `handler` isn't in `handlers_`, right?
++epoch_;Please add a test de-hibernating while the page is hidden, simulating background rendering? I think that's a very important feature of this CL.
// Hibernation may not be pending, we may not be registered.Following up on the other comment about the `DCHECK` in `Unregister`. If my understanding is correct that `Clear()` is only ever called when we `IsHibernating()` is true, it's not true that "Hibernation may not be pending". I'd just remove the comment.
using ::testing::Bool;
using ::testing::Combine;
using ::testing::Values;Move these to line 37, with the other using declarations.
For bonus point (optionally), you could move all 4 using declarations to line 39, right after opening the anonymous namespace. This is recommended in https://abseil.io/tips/119:
โDeclare namespace aliases and using-declarations inside the innermost namespace, whether named or anonymous. (Do not add an anonymous namespace just for this purpose.)โ
task_environment_.FastForwardBy(
CanvasHibernationHandler::kBeforeCompressionDelay - delay -
base::Seconds(1));Is this statement needed? Wouldn't it be equivalent to do the following?
```
auto delay = WaitForHibernation();
EXPECT_TRUE(handler.IsHibernating());
SetPageVisible(&delegate, &handler, platform, true);
task_environment_.FastForwardBy(
CanvasHibernationHandler::kBeforeCompressionDelay - delay);
```
auto task_runner = base::MakeRefCounted<TestSingleThreadTaskRunner>();Benoit LizeShouldn't the test do `handler.SetBackgroundTaskRunnerForTesting(task_runner);`? If this works without, why are all other tests doing it? Could you remove that from other tests?
Done
So, why isn't `SetBackgroundTaskRunnerForTesting` needed here? Is it needed in other tests?
TEST_P(CanvasHibernationHandlerTest, ForegroundFlipForBeforeEncoding) {Why removing this test? It looked useful. `ForegroundFlipFlopDuringCompression` tests toggling visibility after compression but before the OnEncoded callback, whereas this test was toggling visibility before encoding. Isn't it?
ForegroundAfterHibernationBeforeCompression) {How is this test different than `ForegroundBeforeCompression`? Both foreground the page after hibernation and before compression.
ForegroundAfterHibernationBeforeCompression) {There's one test missing. We are testing:
- foregrounding before hibernation starts
- foregrounding after hibernation but before compression.
We are not however testing foregrounding **during** encoding, that is, after compression but before the `OnEncoded` callback is invoked. There's a FlipFlopDuringCompression, but I suppose that's a different use case because that's testing the epoch logic.
TEST_P(CanvasHibernationHandlerTest, ForegroundFlipFlopDuringCompression) {Shouldn't there also be a test flip-flopping before hibernation?
<enum name="CanvasHibernationEvent">
<int value="0" label="Scheduled"/>
<int value="1"
label="Aborted due to destruction while hibernate was pending"/>
<int value="2" label="Aborted due to pending destruction (obsolete)"/>
<int value="3" label="Aborted due to visibility change"/>
<int value="4" label="Aborted due to GPU Context loss"/>
<int value="5" label="Aborted due to switch to unaccelerated rendering"/>
<int value="6" label="Aborted due to allocation failure (obsolete)"/>
<int value="7" label="Aborted due to snapshot failure"/>
<int value="8" label="Ended normally"/>
<int value="9" label="Ended with switch to background rendering"/>
<int value="10" label="Ended with fallback to software"/>
<int value="11" label="Ended with teardown"/>
<int value="12" label="Aborted because there was no surface"/>
<int value="13"
label="Aborted because of epoch mismatch (e.g. fg/bg transitions)"/>
</enum>Hum. Does this mean that the numbers we are seeing in UMA aren't label correctly? Have I been interpreting the numbers wrong this whole time? That's a bummer.
Looking closer, I realized that the histograms for normal vs. background rendering aren't even recorded correctly. I sent https://crrev.com/c/6968030 to fix this.
Related details
- Benoit Lize
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benoit Lize (Gerrit)
Benoit Lize added 20 comments![]( "Open in Gerrit")
Thanks!
Apologies for the very long delay, getting back to it now.
Please keep features sorted by identifier name. `kCanvas2DHibernationDefer` should be placed after `kCanvas2DHibernation` to maintain alphabetical order. (Blink Style Guide: Feature definitions and associated constants)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your ๐ feedback ๐ to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kCanvas2DHibernationDefer);Please keep feature declarations sorted by identifier name. `kCanvas2DHibernationDefer` should be placed after `kCanvas2DHibernation` to maintain alphabetical order. (Blink Style Guide: Feature declarations and associated constants)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your ๐ feedback ๐ to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
Jean-Philippe Gravelnit: Per the style guide, obvious parameter names may be omitted from function declarations in header files. Consider removing the names for `image`, `recorder`, and `context` as their purpose is clear from their types. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your ๐ feedback ๐ to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
The style guide says "may omit", which I take as an optional recommendation. FWIW, I'm not a fan of this guideline because it prevents clang-tidy from validating `/*parameter_name=*/` comments at call sites. Here, `recorder` is a pointer, so what if a caller ever wanted to do `/*recorder=*/nullptr`? That doesn't really make sense in this particular example, but it goes to show that this general guideline is flawed.
I wonder if this recommendation is still up to date. Maybe it predates clang-tidy's check?
In any case, don't mind me. Feel free to follow the recommendation if you want. It's in the style guide! ;)
Yes, I also prefer keeping parameter names, for the same reason. Keeping it for consistency with the rest of the code as well.
kMaxValue = kHibernationAbortedBecauseNoSurface,Benoit LizeUpdate kMaxValue.
Done
Benoit LizeI think that there's something missing here. Originally, we called `HibernateOrLogFailure` on an idle task and this task ended up setting `image_`. From this point, `IsHibernating()` would return true. `IsHibernating` is what we check to know we must wake up from hibernation [if the page does background rendering](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc;l=1457-1485;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18), of if [the page becomes visible again](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc;l=1040-1042;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18). Because `HibernateOrLogFailure` was called as soon as idle (hopefully before the next JavaScript task, but maybe not?), `IsHibernating()` returned mostly the right value.
With the new code though, `IsHibernating()` will only start to return `true` after several minutes. This leaves plenty of time of the page to restart rendering in the background (82% of hibernation cases). If this happens, `Hibernate()` will run unimpeded because the page is still hidden. But that will hibernate the canvas while it's being used in the background.
Jean-Philippe GravelI guess there are a couple solutions here: we could either give up on hibernation when rendering happens in background, or only hibernate if the canvas hasn't been rendering for a while.
The first one is more conservative and easier to implement, by bumping `epoch_` in `Clear()`. This is what I've done in this CL. wdyt?
Increasing `epoch_` sounds reasonable. But I wonder what will happen if you toggle between tabs a lot. Say you have many tabs each with lots of canvases and you ctrl-tab constantly for 5 minutes. How many pending tasks will accumulate? Can this be a problem? Is there a way of actively cancelling tasks?
The cost of a single task is ~2-3us, and looking at the implementation of CancellableTask, is fairly similar to what's being done here, possibly more expensive, on top of being less easy to use (since you need to keep a tab on all the TaskHandle objects).
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/scheduler/public/post_cancellable_task.h;drc=aff3b5b1f9a377ec59b599200d2fbe6611941361;l=78
So I am not very worried about the cost (I don't think a user can switch tabs more than every 100ms, and even with 100 canvas elements per page, this would still be 0.01% of CPU time), and think this is the simpler approach.
DCHECK(handlers_.Contains(handler));Benoit LizeWhat changed in this CL for `Unregister` to be called when `Register` wasn't?
Jean-Philippe GravelAdded a comment in the code.
You mean this comment?
```
// Hibernation may not be pending, we may not be registered.
```
Comments should explains the why, but not the what. I still understand how it's possible for `Clear()` to be called if `Register()` wasn't. All calls to `Clear()` are guarded by `if (IsHibernating())` checks and `IsHibernating()` is only `true` if `Register()` is called. Isn't it?That being said, I suppose we are better off without the `DCHECK` regardless... It's not protecting against anything harmful. `handlers_.erase(handler);` is a no-op if `handler` isn't in `handlers_`, right?
I think this is true, but wasn't certain enough, and as you say, it's a noop if the handler is not registered.
Please add a test de-hibernating while the page is hidden, simulating background rendering? I think that's a very important feature of this CL.
This is already tested, added a comment.
// Hibernation may not be pending, we may not be registered.Following up on the other comment about the `DCHECK` in `Unregister`. If my understanding is correct that `Clear()` is only ever called when we `IsHibernating()` is true, it's not true that "Hibernation may not be pending". I'd just remove the comment.
Done
using ::testing::Bool;
using ::testing::Combine;
using ::testing::Values;Move these to line 37, with the other using declarations.
For bonus point (optionally), you could move all 4 using declarations to line 39, right after opening the anonymous namespace. This is recommended in https://abseil.io/tips/119:
โDeclare namespace aliases and using-declarations inside the innermost namespace, whether named or anonymous. (Do not add an anonymous namespace just for this purpose.)โ
Done
using ::testing::Bool;
using ::testing::Combine;
using ::testing::Values;Move these to line 37, with the other using declarations.
For bonus point (optionally), you could move all 4 using declarations to line 39, right after opening the anonymous namespace. This is recommended in https://abseil.io/tips/119:
โDeclare namespace aliases and using-declarations inside the innermost namespace, whether named or anonymous. (Do not add an anonymous namespace just for this purpose.)โ
Done
task_environment_.FastForwardBy(
CanvasHibernationHandler::kBeforeCompressionDelay - delay -
base::Seconds(1));Is this statement needed? Wouldn't it be equivalent to do the following?
```
auto delay = WaitForHibernation();
EXPECT_TRUE(handler.IsHibernating());
SetPageVisible(&delegate, &handler, platform, true);
task_environment_.FastForwardBy(
CanvasHibernationHandler::kBeforeCompressionDelay - delay);
```
Done
task_environment_.FastForwardBy(
CanvasHibernationHandler::kBeforeCompressionDelay - delay -
base::Seconds(1));Is this statement needed? Wouldn't it be equivalent to do the following?
```
auto delay = WaitForHibernation();
EXPECT_TRUE(handler.IsHibernating());
SetPageVisible(&delegate, &handler, platform, true);
task_environment_.FastForwardBy(
CanvasHibernationHandler::kBeforeCompressionDelay - delay);
```
Done
if (!base::FeatureList::IsEnabled(features::kCanvas2DHibernationDefer)) {
GTEST_SKIP();
}Benoit LizeWhy skipping this test? The original test seemed useful for the normal operation mode. Why drop the test coverage?
Done.
Done
auto task_runner = base::MakeRefCounted<TestSingleThreadTaskRunner>();Benoit LizeShouldn't the test do `handler.SetBackgroundTaskRunnerForTesting(task_runner);`? If this works without, why are all other tests doing it? Could you remove that from other tests?
Jean-Philippe GravelDone
So, why isn't `SetBackgroundTaskRunnerForTesting` needed here? Is it needed in other tests?
The TaskEnvironment runs the thread pool tasks on the same thread as the main thread tasks, and since these tasks are not delayed, they would run all at once. Using an explicit background task runner allows to change things while compression has been posted, but the main thread callback hasn't executed.
It also allows to distinguish the cases of "compression task hasn't been posted" from the "compression happened, the result was discarded".
TEST_P(CanvasHibernationHandlerTest, ForegroundAfterEncoding) {Benoit LizeThe new test version seems to foregrounds **before** encoding, not after. Isn't it essentially testing the same thing as `ForegroundTooEarly` and the new test above? Could you keep logic waiting for the encoding?
Changed the tests, to avoid redundant, and keep the same / better coverage. Thanks for the comment.
Done
TEST_P(CanvasHibernationHandlerTest, ForegroundFlipForBeforeEncoding) {Why removing this test? It looked useful. `ForegroundFlipFlopDuringCompression` tests toggling visibility after compression but before the OnEncoded callback, whereas this test was toggling visibility before encoding. Isn't it?
The background compression task is static, does not acquire locks nor look at the main thread state, so it is not relevant when it runs, which is why I removed this test. I can bring it back, but it felt redundant here.
ForegroundAfterHibernationBeforeCompression) {There's one test missing. We are testing:
- foregrounding before hibernation starts
- foregrounding after hibernation but before compression.We are not however testing foregrounding **during** encoding, that is, after compression but before the `OnEncoded` callback is invoked. There's a FlipFlopDuringCompression, but I suppose that's a different use case because that's testing the epoch logic.
Added this test, I think it is effectively covered by the other test, but you're right, better be explicit.
TEST_P(CanvasHibernationHandlerTest, ForegroundFlipFlopDuringCompression) {Shouldn't there also be a test flip-flopping before hibernation?
This is covered by the ForegroundBackgroundWithDelay test, which tests multiple such transitions, and that the canvas eventually hibernates.
<enum name="CanvasHibernationEvent">
<int value="0" label="Scheduled"/>
<int value="1"
label="Aborted due to destruction while hibernate was pending"/>
<int value="2" label="Aborted due to pending destruction (obsolete)"/>
<int value="3" label="Aborted due to visibility change"/>
<int value="4" label="Aborted due to GPU Context loss"/>
<int value="5" label="Aborted due to switch to unaccelerated rendering"/>
<int value="6" label="Aborted due to allocation failure (obsolete)"/>
<int value="7" label="Aborted due to snapshot failure"/>
<int value="8" label="Ended normally"/>
<int value="9" label="Ended with switch to background rendering"/>
<int value="10" label="Ended with fallback to software"/>
<int value="11" label="Ended with teardown"/>
<int value="12" label="Aborted because there was no surface"/>
<int value="13"
label="Aborted because of epoch mismatch (e.g. fg/bg transitions)"/>
</enum>Hum. Does this mean that the numbers we are seeing in UMA aren't label correctly? Have I been interpreting the numbers wrong this whole time? That's a bummer.
Looking closer, I realized that the histograms for normal vs. background rendering aren't even recorded correctly. I sent https://crrev.com/c/6968030 to fix this.
- Jean-Philippe Gravel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |