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

Texture pages are not properly reused or deallocated #914

Closed
staktrace opened this issue Feb 22, 2017 · 5 comments
Closed

Texture pages are not properly reused or deallocated #914

staktrace opened this issue Feb 22, 2017 · 5 comments
Assignees

Comments

@staktrace
Copy link
Contributor

@staktrace staktrace commented Feb 22, 2017

When running the gecko webgl mochitest suite, WR memory usage climbs into multiple gigabytes (see https://bugzilla.mozilla.org/show_bug.cgi?id=1341745). I traced this down to the fact that WR periodically allocates a new TexturePage but never goes back and reuses old texture pages or deallocates them if they are empty. I checked with @kvark and he agreed that this appears to be a bug. Quoting him from IRC:

I don't see TexturePage objects being de-allocated. Also, they are not re-used properly. As in -
allocation only checks for the last page, while other pages may have more free space if they got
some content removed from them
so I think 2 modifications need to be done: 1) removal of the last page in a vector if it becomes
empty, and 2) check for all the pages when allocating

@kvark kvark self-assigned this Feb 22, 2017
@kvark
Copy link
Member

@kvark kvark commented Feb 22, 2017

Note the comment:

// TODO(gw): Actually free items from the texture cache!!

@glennw
Copy link
Member

@glennw glennw commented Feb 22, 2017

committed on 29 Oct 2015

I guess it's time to fix that!

@kvark
Copy link
Member

@kvark kvark commented Feb 22, 2017

I guess I shouldn't try to actually remove the TexturePage from the arena then? Since doing so would potentially allow for a test case to constantly create and destroy textures each frame, and the real reason for the leak is that 2015 missing feature.

@glennw
Copy link
Member

@glennw glennw commented Feb 22, 2017

It'd probably be worth adding some profile counters that track some of the texture cache stats (e.g. total bytes allocated, page count etc) so that we can visually see if something is leaking in the future.

bors-servo added a commit that referenced this issue Feb 27, 2017
Texture allocation refactoring

Fixes #914

Note: the [comment](#914 (comment)) appears to be irrelevant, since the actual texture freeing happens in `texture_cache`, and the free list just contains texture items, which are plain old data (PODs).

@staktrace I suggest we proceed with this PR and check if it resolves your issue, then potentially come with a follow-up.
@glennw r?

<!-- 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/919)
<!-- Reviewable:end -->
@staktrace
Copy link
Contributor Author

@staktrace staktrace commented Feb 27, 2017

@kvark thanks, the fix seems to work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.