Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

delete FontInstances from FontContexts when evicting a GlyphCache #3177

Merged
merged 1 commit into from Oct 9, 2018

Conversation

@lsalzman
Copy link
Contributor

lsalzman commented Oct 9, 2018

This is to address Gecko bug https://bugzilla.mozilla.org/show_bug.cgi?id=1495661

If we have a page that is animating the variation values within a font, this can cause continuous accumulation of new FontInstances in the FontContext. These will never get cleared out until we delete the FontKey from which they were generated. This may never happen until the tab or window is closed, even though Gecko itself has long ago deleted the corresponding ScaledFonts to and told WR to delete the FontInstanceKeys.

Due to the fact that zoom and device scale can cause a FontInstanceKey to no longer map to the FontInstance used to key the GlyphCache and the FontContext, there can be multiple FontInstances with sizes that can't be traced back to the original FontInstanceKey. This prevents us from doing a straightforward deletion based on the value immediately stored with the FontInstanceKey.

However, we already have a mechanism in place to deal with this in the GlyphCache, evicting GlyphKeyCaches that have also been evicted from the TextureCache, so that we have effective limiting in place of FontInstances that haven't been recently used. So all we need to do here is connect that to deleting the FontInstance in the FontContext when it is being evicted from the GlyphCache. In the worst cases this may cause a false positive that evicts something from the FontContext while it is still alive, but it can be rematerialized when necessary, and this should be relatively rare. This will also give us a natural hard limit on the number of backend API fonts we keep around in this scenario.


This change is Reviewable

@gw3583
gw3583 approved these changes Oct 9, 2018
Copy link
Collaborator

gw3583 left a comment

Reviewed 6 of 6 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Oct 9, 2018

The change looks good. Do we know if this fixes the reported issue, or do we need to get it into gecko and see? And do we need a try run for this?

@lsalzman

This comment has been minimized.

Copy link
Contributor Author

lsalzman commented Oct 9, 2018

I asked for Jonathan to try this out locally to see if he can reproduce the issue. We'll see how it goes.

This shouldn't have any visual effect on how things are rendered, so I wouldn't expect a try run say much here.

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Oct 9, 2018

I'll add delegate to this so it can be merged if / when you hear back that it fixes the reported issue.

@bors-servo delegate+

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Oct 9, 2018

✌️ @lsalzman can now approve this pull request

@lsalzman

This comment has been minimized.

Copy link
Contributor Author

lsalzman commented Oct 9, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Oct 9, 2018

📌 Commit a0af8b0 has been approved by lsalzman

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Oct 9, 2018

⌛️ Testing commit a0af8b0 with merge 923ee49...

bors-servo added a commit that referenced this pull request Oct 9, 2018
delete FontInstances from FontContexts when evicting a GlyphCache

This is to address Gecko bug https://bugzilla.mozilla.org/show_bug.cgi?id=1495661

If we have a page that is animating the variation values within a font, this can cause continuous accumulation of new FontInstances in the FontContext. These will never get cleared out until we delete the FontKey from which they were generated. This may never happen until the tab or window is closed, even though Gecko itself has long ago deleted the corresponding ScaledFonts to and told WR to delete the FontInstanceKeys.

Due to the fact that zoom and device scale can cause a FontInstanceKey to no longer map to the FontInstance used to key the GlyphCache and the FontContext, there can be multiple FontInstances with sizes that can't be traced back to the original FontInstanceKey. This prevents us from doing a straightforward deletion based on the value immediately stored with the FontInstanceKey.

However, we already have a mechanism in place to deal with this in the GlyphCache, evicting GlyphKeyCaches that have also been evicted from the TextureCache, so that we have effective limiting in place of FontInstances that haven't been recently used. So all we need to do here is connect that to deleting the FontInstance in the FontContext when it is being evicted from the GlyphCache. In the worst cases this may cause a false positive that evicts something from the FontContext while it is still alive, but it can be rematerialized when necessary, and this should be relatively rare. This will also give us a natural hard limit on the number of backend API fonts we keep around in this scenario.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3177)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Oct 9, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: lsalzman
Pushing 923ee49 to master...

@bors-servo bors-servo merged commit a0af8b0 into servo:master Oct 9, 2018
4 checks passed
4 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
code-review/reviewable 6 files reviewed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.