Avoid calling Dart_HintFreed more than once every five seconds #25827
Conversation
|
Will platform low-memory signals override the 5-second delay? |
|
Platform low memory signals end up calling |
|
Hm. I might have a better solution framework side that would let us just not call Dart_HintFreed when images are released anyway. |
|
Ahhhh this failure isn't a flake haha. We call hint freed less often now. Argh. I'll have to think about this some more probably on monday. |
|
Ok - the change here is to make sure we don't reset the timer if we didn't actually have any bytes to hint freeing, since that also is effectively a no-op. |
| @@ -299,4 +302,44 @@ TEST_F(EngineTest, PassesLoadDartDeferredLibraryErrorToRuntime) { | |||
| }); | |||
| } | |||
|
|
|||
| TEST_F(EngineTest, NotifyIdleMoreThanOncePerFiveSeconds) { | |||
There was a problem hiding this comment.
Don't you want to show a timedelta that is less that 5 seconds so you can show it doesn't get called?
There was a problem hiding this comment.
I'm not sure I have it clear in my head about which case you're asking for.
This case is covering three calls, at 10s, 13s, and 16s. We're verifying that NotifyIdle only gets called twice: the very first call and the last one.
| std::make_unique<MockRuntimeController>(client, task_runners_); | ||
|
|
||
| EXPECT_CALL(*mock_runtime_controller, NotifyIdle(200, 100)).Times(1); | ||
| EXPECT_CALL(*mock_runtime_controller, NotifyIdle(400, 0)).Times(1); |
There was a problem hiding this comment.
So this line here is verifying that we didn't get called (the second param is 0) at the 13s mark.
There was a problem hiding this comment.
Ah yea, a bit weird to follow since mocking and calling are in different spaces and it isn't straitforward. You might want to add a comment like:
Here we assert NotifyIdle is called with the zero hint since we haven't advanced 5 (or name of timeout constant) seconds yet.
There was a problem hiding this comment.
I can do that - unfortunately it's this way because we std::move the pointer after this, and I didn't want to grab out a copy of the unique_ptr.
|
I've filed flutter/flutter#81514, since that would be an even better fix. If we could really do that, we should be able to drop the whole images notify the VM to maybe do a GC to clean themselves up. |
|
https://github.com/flutter/flutter/issues/80702 I hope it can be merged as soon as possible, which brings us great distress and brings bad experience to users. |
|
@DizzyYang this patch has been merged, and it has rolled into the framework. I'm working on an additional follow up that will do a better job in this case, see flutter/flutter#84740 I'm not aware of any plans to CP these fixes into stable though, if that's what you're looking for. |
|
@dnfield Thank you for your reply. [✓] Flutter (Channel stable, 2.2.2, on macOS 11.4 20F71 darwin-x64, locale zh-Hans-CN) This is the version of SDK we are using. In this version, the CPU still has a big problem. This version should not be merged yet, and I hope it can be merged as soon as possible. |
|
No. |
This is to fix flutter/flutter#80702
I believe this approach makes sense because:
Dart_HintFreedhas no effect until the nextDart_NotifyIdlecall.I picked 5 seconds somewhat arbitrarily. It still means we'd be running a GC at least every five seconds when animating a gif.
I created a devicelab benchmark for this, but it isn't quite reporting the right values to Skia perf yet. I will fix that before marking this as ready for review, but wanted to get feedback on the approach here in the mean time.
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.