Trigger cleanup, once rendering has finished, in PDFThumbnailView.draw
#12613
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… 10217 follow-up)
This patch will help reduce memory usage, especially for longer documents, when the user scrolls around in the thumbnailView (in the sidebar). Note how the `PDFPageProxy.cleanup` method will, assuming it's safe to do so, release main-thread resources associated with the page. These include things such as e.g. image data (which can be arbitrarily large), and also the operatorList (which can also be quite large). Hence when pages are evicted from the `PDFPageViewBuffer`, on the `BaseViewer`-instance, the `PDFPageView.destroy` method is invoked which will (among other things) call `PDFPageProxy.cleanup` in the API. However, looking at the `PDFThumbnailViewer`/`PDFThumbnailView` classes you'll notice that there's no attempt to ever call `PDFPageProxy.cleanup`, which implies that in certain circumstances we'll essentially keep all resources allocated permanently on the `PDFPageProxy`-instances in the API. In particular, this happens when the users opens the sidebar and starts scrolling around in the thumbnails. Generally speaking you obviously need to keep all thumbnail *images* around, since otherwise the thumbnailView is useless, but there's still room for improvement here. Please note that the case where a *rendered page* is used to create the thumbnail is (obviously) completely unaffected by the issues described above, and this rather only applies to thumbnails being explicitly rendered by the `PDFThumbnailView.draw` method. For the latter case, we can fix these issues simply by calling `PDFPageProxy.cleanup` once rendering has finished. To prevent *accidentally* pulling the rug out from under `PDFPageViewBuffer` in the viewer, which expects data to be available, this required adding a couple of new methods[1] to enable checking that it's indeed safe to call `PDFPageProxy.cleanup` from the `PDFThumbnailView.draw` method. It's really quite fascinating that no one has noticed this issue before, since it's been around since basically "forever". --- [1] While it should be *very* rare for `PDFThumbnailView.draw` to be called for a pageView that's also in the `PDFPageViewBuffer`, given that pages are rendered before thumbnails and that the *rendered page* is used to create the thumbnail, it can still happen since rendering is asynchronous. Furthermore, it's also possible for `PDFThumbnailView.setImage` to be disabled, in which case checking the `PDFPageViewBuffer` for active pageViews *really* matters.
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/15fd49953e7eaf4/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/15fd49953e7eaf4/output.txt Total script time: 4.07 mins Published |
timvandermeij
approved these changes
Nov 12, 2020
|
Interesting find; thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch will help reduce memory usage, especially for longer documents, when the user scrolls around in the thumbnailView (in the sidebar).
Note how the
PDFPageProxy.cleanupmethod will, assuming it's safe to do so, release main-thread resources associated with the page. These include things such as e.g. image data (which can be arbitrarily large), and also the operatorList (which can also be quite large).Hence when pages are evicted from the
PDFPageViewBuffer, on theBaseViewer-instance, thePDFPageView.destroymethod is invoked which will (among other things) callPDFPageProxy.cleanupin the API.However, looking at the
PDFThumbnailViewer/PDFThumbnailViewclasses you'll notice that there's no attempt to ever callPDFPageProxy.cleanup, which implies that in certain circumstances we'll essentially keep all resources allocated permanently on thePDFPageProxy-instances in the API.In particular, this happens when the users opens the sidebar and starts scrolling around in the thumbnails. Generally speaking you obviously need to keep all thumbnail images around, since otherwise the thumbnailView is useless, but there's still room for improvement here.
Please note that the case where a rendered page is used to create the thumbnail is (obviously) completely unaffected by the issues described above, and this rather only applies to thumbnails being explicitly rendered by the
PDFThumbnailView.drawmethod.For the latter case, we can fix these issues simply by calling
PDFPageProxy.cleanuponce rendering has finished. To prevent accidentally pulling the rug out from underPDFPageViewBufferin the viewer, which expects data to be available, this required adding a couple of new methods[1] to enable checking that it's indeed safe to callPDFPageProxy.cleanupfrom thePDFThumbnailView.drawmethod.It's really quite fascinating that no one has noticed this issue before, since it's been around since basically "forever".
[1] While it should be very rare for
PDFThumbnailView.drawto be called for a pageView that's also in thePDFPageViewBuffer, given that pages are rendered before thumbnails and that the rendered page is used to create the thumbnail, it can still happen since rendering is asynchronous.Furthermore, it's also possible for
PDFThumbnailView.setImageto be disabled, in which case checking thePDFPageViewBufferfor active pageViews really matters.