compositing: Drop Painters when they have no WebViews#41144
compositing: Drop Painters when they have no WebViews#41144mukilan merged 1 commit intoservo:mainfrom
Painters when they have no WebViews#41144Conversation
servo-highfive
added
the
S-awaiting-review
❌ 5 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
mrobinson
force-pushed
the
cleanup-painters
branch
from
16d933a to
d7e0534
Compare
mukilan
left a comment
mukilan
left a comment
There was a problem hiding this comment.
The description says
We don't really need to do this for Compositor API calls because they are triggered via the WebView and there should be no WebViews for the Painter left. The expect() call still makes sense in that case.
I guess for the case of compositor API calls, we expect at least one WebView in the Painter instead of "there should be no WebViews for the Painter left."
| ) { | ||
| let painter_id = webview_id.into(); | ||
| let painter = self.maybe_painter(painter_id); | ||
| let image_keys = (0..pref!(image_key_batch_size)) |
There was a problem hiding this comment.
Is there any reason we need to return dummy image keys rather than just an empty vector?
There was a problem hiding this comment.
Yes. If the ImageCache is blocked waiting on image keys it could deadlock the ScriptThread that is using it. The dummy key ensures that if this is the case the ScriptThread can finish what it was doing, no matter what order things happen.
| || ImageKey::new(painter_id.into(), 0), | ||
| |painter| painter.webrender_api.generate_image_key(), | ||
| ); | ||
| let _ = result_sender.send(image_key); |
There was a problem hiding this comment.
Any reason we need to send a dummy image key, instead of ignoring the request? Since the only reference to sender will be dropped, the receiving end should unblock with an Err, which is transformed into an None in generate_image_key_blocking. I'm worried introducing dummy keys might make it tricky to debug issues.
There was a problem hiding this comment.
I think it's better to let the ScriptThread finish execution properly rather than causing a potential panic...this could happen do to ordering issues that are hard to reason about. These ImageKeys will never be used anyway so it is safe to return dummy values.
| self.disabled_lcp_for_webviews.insert(webview_id); | ||
| } | ||
|
|
||
| pub(crate) fn clear(&mut self) { |
There was a problem hiding this comment.
Why are we removing this method, instead of invoking it from the Drop impl of Painter? It is currently being called when Painter is deinited, so shouldn't we retain that behaviour? It would also mean disabled_lcp_for_webviews grows unbounded as it never gets cleared anywhere else AFAICT.
There was a problem hiding this comment.
Oh, I looked at the code again and the LCP calculator is part of the Painter's state (and not part of Compositor as I originally assumed), so I guess we don't need to destroy it explicitly. However, it still seems like removing a webview from the painter should also remove it from disabled_lcp_for_webviews.
There was a problem hiding this comment.
That makes sense. I'll make that change. EDIT: And yeah, to confirm this was just clearing vectors during deinit which happens before Drop anyway, so it was doing useless work.
There was a problem hiding this comment.
I've gone ahead and done this.
Remove `Painter`s from the `Compositor` when their final `WebView` is dropped. This ensures that their resources are properly released and deinitialized, such as the WebRender instance and all of its resources. Without this change, these things leak. This change requires the ability to handle inexistant `Painter`s during message handling, as messages from other parts of the `Constellation` for a `Painter` can arrive after it has been released. We don't really need to do this for `Compositor` API calls because they are triggered via the `WebView` and there should be no `WebView`s for the `Painter` left. The `expect()` call still makes sense in that case. Part of this also involves removing the redundant `RemoveWebView` message to the `Compositor`. This is handled now in the `WebView` `Drop` implementation for both simplicity and necessity. Signed-off-by: Martin Robinson <[email protected]>
mrobinson
force-pushed
the
cleanup-painters
branch
from
d7e0534 to
819f74c
Compare
My original wording was really confusing. I've reworded this to be a bit clearer. Thanks for the review, @mukilan. I think I've responded to all your review comments. PTAL. |
servo-highfive
removed
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
4 participants
Remove
Painters from theCompositorwhen their finalWebViewisdropped. This ensures that their resources are properly released and
deinitialized, such as the WebRender instance and all of its resources.
Without this change, these things leak.
This change requires the ability to handle inexistant
Painters duringmessage handling, as messages from other parts of the
Constellationfor a
Paintercan arrive after it has been released. We don't reallyneed to do this for
CompositorAPI calls because they are triggeredvia the
WebView. When aWebViewis alive itsPaintershould alwaysalso be alive, so the
expect()call still makes sense in that case.Part of this also involves removing the redundant
RemoveWebViewmessage to the
Compositor. This is handled now in theWebViewDropimplementation for both simplicity and necessity.
Testing: This should not change observable behavior, but does fix a memory leak
while closing windows.