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

Zero the height and width of the PageView canvas before deleting. #4920

Merged

Conversation

nnethercote
Copy link
Contributor

I've been measuring a 122 page document. Scrolling through it fairly quickly (i.e. at a speed where every page just has time to render) on my Mac, Firefox's memory usage jumps up to about 2,300 MiB, and most of it is canvas pixels.

PageView.destroy() actually deletes canvases for pages that fall out of the cache, but GC doesn't necessarily happen immediately, which means the pixel data can be held on to for a while. Robert O'Callahan told me that if you zero the |width| and |height| of a canvas, in Firefox the graphics resources are freed immediately.

This patch does this, and for the scenario described above it reduces peak memory usage from ~2,300 MiB to between ~1,000 and ~1,400 MiB. Not bad.

(If I scroll through the document really fast, then PageView.destroy() doesn't get called on most of the pages, and so this patch doesn't help much. Working out why destroy() isn't being called in that scenario can be done in a follow-up.)

@Snuffleupagus
Copy link
Collaborator

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.21.233.14:8877/954278025c870c3/output.txt

yurydelendik added a commit that referenced this pull request Jun 12, 2014
Zero the height and width of the PageView canvas before deleting.
@yurydelendik yurydelendik merged commit 12a35f2 into mozilla:master Jun 12, 2014
@yurydelendik
Copy link
Contributor

Thank you for the patch

@nnethercote nnethercote deleted the zero-canvas-before-deleting branch June 13, 2014 15:37
@anthonyryan1
Copy link

Is there a reason Firefox doesn't release the resources when the canvas has been deleted? Or has a bug report been filed about this already.

That feels like the real bug and this looks like more of a workaround to me.

@yurydelendik
Copy link
Contributor

Is there a reason Firefox doesn't release the resources

I don't think it's specific to Firefox. And as it is explained above, graphics resources will be released on next GC turn.

@nnethercote
Copy link
Contributor Author

Is there a reason Firefox doesn't release the resources when the canvas has been deleted?

In JavaScript, "delete" means "remove a property from an object". In C++, "delete" means "destruct an object and free its memory". My description is using the JavaScript meaning, but I admit it is unclear and potentially confusing.

So what happens is that we delete the |canvas| property from the PageView object. This is the only reference to the canvas object, and so the canvas is now garbage and the GC will collect it next time it runs. And when it's collected, the graphics resources on the C++ side of the implementation will be freed.

However, because we have application-specific knowledge that the JS engine does not, we know that this is the last reference to the canvas object. Thus we can zero the height and width and the C++ resources will be freed immediately. It's effectively a little bit of manual memory management in a language that doesn't generally allow it, made possible due to our knowledge of Firefox's internals. And in this case it's really effective because canvases are large and created quickly while scrolling, so even a small delay in collecting them can hurt.

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

Successfully merging this pull request may close these issues.

5 participants