Skip to content

Commit

Permalink
Trigger cleanup, once rendering has finished, in PDFThumbnailView.draw
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Snuffleupagus committed Nov 12, 2020
1 parent 8b5bc8d commit 4a9994b
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 0 deletions.
30 changes: 30 additions & 0 deletions web/base_viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@ function PDFPageViewBuffer(size) {
data.shift().destroy();
}
};

this.has = function (view) {
return data.includes(view);
};
}

function isSameScale(oldScale, newScale) {
Expand Down Expand Up @@ -1113,6 +1117,32 @@ class BaseViewer {
});
}

/**
* @param {number} pageNumber
*/
isPageCached(pageNumber) {
if (!this.pdfDocument || !this._buffer) {
return false;
}
if (
!(
Number.isInteger(pageNumber) &&
pageNumber > 0 &&
pageNumber <= this.pagesCount
)
) {
console.error(
`${this._name}.isPageCached: "${pageNumber}" is not a valid page.`
);
return false;
}
const pageView = this._pages[pageNumber - 1];
if (!pageView) {
return false;
}
return this._buffer.has(pageView);
}

cleanup() {
for (let i = 0, ii = this._pages.length; i < ii; i++) {
if (
Expand Down
5 changes: 5 additions & 0 deletions web/interfaces.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,11 @@ class IPDFLinkService {
* @param {number} pageNumber
*/
isPageVisible(pageNumber) {}

/**
* @param {number} pageNumber
*/
isPageCached(pageNumber) {}
}

/**
Expand Down
14 changes: 14 additions & 0 deletions web/pdf_link_service.js
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,13 @@ class PDFLinkService {
isPageVisible(pageNumber) {
return this.pdfViewer.isPageVisible(pageNumber);
}

/**
* @param {number} pageNumber
*/
isPageCached(pageNumber) {
return this.pdfViewer.isPageCached(pageNumber);
}
}

function isValidExplicitDestination(dest) {
Expand Down Expand Up @@ -609,6 +616,13 @@ class SimpleLinkService {
isPageVisible(pageNumber) {
return true;
}

/**
* @param {number} pageNumber
*/
isPageCached(pageNumber) {
return true;
}
}

export { PDFLinkService, SimpleLinkService };
10 changes: 10 additions & 0 deletions web/pdf_thumbnail_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,16 @@ class PDFThumbnailView {
finishRenderTask(error);
}
);
// Only trigger cleanup, once rendering has finished, when the current
// pageView is *not* cached on the `BaseViewer`-instance.
resultPromise.finally(() => {
const pageCached = this.linkService.isPageCached(this.id);
if (pageCached) {
return;
}
this.pdfPage?.cleanup();
});

return resultPromise;
}

Expand Down

0 comments on commit 4a9994b

Please sign in to comment.