From 9241e1be8c0fb74a13bf110e172c64de2b319149 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 22 Jan 2025 11:02:16 +0100 Subject: [PATCH] [api-minor] Simplify clean-up of page resources after rendering After PR 2317, which landed in 2012, we'd immediately clean-up after rendering for pages with large image resources. This had the effect that re-rendering, e.g. after zooming, would force us to re-parse the entire page which could easily lead to bad performance. In PR 16108, which landed in 2023, we tried to lessen the impact of that by slightly delaying clean-up however that's obviously not a perfect solution (and it increased the complexity of the relevant code). Furthermore, the condition for this "immediate" clean-up seems a bit arbitrary to me since a page could easily contain a large number of smaller images whose total size vastly exceeds the threshold. Hence this patch, which suggests that we remove the conditional and delayed clean-up after rendering. Compared to the situation back in 2012, a number of things have improved since: - We have *multiple* caches for repeated image-resources on the worker-thread[1], which helps reduce overall memory usage and improves performance. - We downsize huge images on the worker-thread, which means that the images we're using on the main-thread cannot be arbitrarily large. - The amount of available RAM on devices should be a lot higher, since more than a decade has passed. A future improvement here, for more resource constrained environments, could be to instead clean-up when actually needed using e.g. `WeakRef`s (see issue 18148). --- [1] More specifically: - `LocalImageCache`, which caches image-data by /Name and /Ref on the `PartialEvaluator.prototype.getOperatorList` level. - `RegionalImageCache`, which caches image-data by /Ref on the `PartialEvaluator`-instance (i.e. at the page) level. - `GlobalImageCache`, which caches image-data by /Ref globally at the document level. --- src/core/image_utils.js | 9 ++------ src/display/api.js | 49 ++++++----------------------------------- src/shared/util.js | 3 --- 3 files changed, 9 insertions(+), 52 deletions(-) diff --git a/src/core/image_utils.js b/src/core/image_utils.js index 9638d37ef..05e2b7b56 100644 --- a/src/core/image_utils.js +++ b/src/core/image_utils.js @@ -13,12 +13,7 @@ * limitations under the License. */ -import { - assert, - MAX_IMAGE_SIZE_TO_CACHE, - unreachable, - warn, -} from "../shared/util.js"; +import { assert, unreachable, warn } from "../shared/util.js"; import { RefSet, RefSetCache } from "./primitives.js"; class BaseLocalCache { @@ -179,7 +174,7 @@ class GlobalImageCache { static MIN_IMAGES_TO_CACHE = 10; - static MAX_BYTE_SIZE = 5 * MAX_IMAGE_SIZE_TO_CACHE; + static MAX_BYTE_SIZE = 5e7; // Fifty megabytes. #decodeFailedSet = new RefSet(); diff --git a/src/display/api.js b/src/display/api.js index 8bc228dd5..0c3a3e2eb 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -25,7 +25,6 @@ import { getVerbosityLevel, info, isNodeJS, - MAX_IMAGE_SIZE_TO_CACHE, RenderingIntentFlag, setVerbosityLevel, shadow, @@ -72,7 +71,6 @@ import { XfaText } from "./xfa_text.js"; const DEFAULT_RANGE_CHUNK_SIZE = 65536; // 2^16 = 65536 const RENDERING_CANCELLED_TIMEOUT = 100; // ms -const DELAYED_CLEANUP_TIMEOUT = 5000; // ms /** * @typedef { Int8Array | Uint8Array | Uint8ClampedArray | @@ -1339,8 +1337,6 @@ class PDFDocumentProxy { * Proxy to a `PDFPage` in the worker thread. */ class PDFPageProxy { - #delayedCleanupTimeout = null; - #pendingCleanup = false; constructor(pageIndex, pageInfo, transport, pdfBug = false) { @@ -1353,7 +1349,6 @@ class PDFPageProxy { this.commonObjs = transport.commonObjs; this.objs = new PDFObjects(); - this._maybeCleanupAfterRender = false; this._intentStates = new Map(); this.destroyed = false; } @@ -1490,10 +1485,8 @@ class PDFPageProxy { ); const { renderingIntent, cacheKey } = intentArgs; // If there was a pending destroy, cancel it so no cleanup happens during - // this call to render... + // this call to render. this.#pendingCleanup = false; - // ... and ensure that a delayed cleanup is always aborted. - this.#abortDelayedCleanup(); optionalContentConfigPromise ||= this._transport.getOptionalContentConfig(renderingIntent); @@ -1532,10 +1525,10 @@ class PDFPageProxy { // Attempt to reduce memory usage during *printing*, by always running // cleanup immediately once rendering has finished. - if (this._maybeCleanupAfterRender || intentPrint) { + if (intentPrint) { this.#pendingCleanup = true; } - this.#tryCleanup(/* delayed = */ !intentPrint); + this.#tryCleanup(); if (error) { internalRenderTask.capability.reject(error); @@ -1769,7 +1762,6 @@ class PDFPageProxy { } this.objs.clear(); this.#pendingCleanup = false; - this.#abortDelayedCleanup(); return Promise.all(waitOn); } @@ -1783,7 +1775,7 @@ class PDFPageProxy { */ cleanup(resetStats = false) { this.#pendingCleanup = true; - const success = this.#tryCleanup(/* delayed = */ false); + const success = this.#tryCleanup(); if (resetStats && success) { this._stats &&= new StatTimer(); @@ -1793,25 +1785,12 @@ class PDFPageProxy { /** * Attempts to clean up if rendering is in a state where that's possible. - * @param {boolean} [delayed] - Delay the cleanup, to e.g. improve zooming - * performance in documents with large images. - * The default value is `false`. * @returns {boolean} Indicates if clean-up was successfully run. */ - #tryCleanup(delayed = false) { - this.#abortDelayedCleanup(); - + #tryCleanup() { if (!this.#pendingCleanup || this.destroyed) { return false; } - if (delayed) { - this.#delayedCleanupTimeout = setTimeout(() => { - this.#delayedCleanupTimeout = null; - this.#tryCleanup(/* delayed = */ false); - }, DELAYED_CLEANUP_TIMEOUT); - - return false; - } for (const { renderTasks, operatorList } of this._intentStates.values()) { if (renderTasks.size > 0 || !operatorList.lastChunk) { return false; @@ -1823,13 +1802,6 @@ class PDFPageProxy { return true; } - #abortDelayedCleanup() { - if (this.#delayedCleanupTimeout) { - clearTimeout(this.#delayedCleanupTimeout); - this.#delayedCleanupTimeout = null; - } - } - /** * @private */ @@ -1863,7 +1835,7 @@ class PDFPageProxy { } if (operatorListChunk.lastChunk) { - this.#tryCleanup(/* delayed = */ true); + this.#tryCleanup(); } } @@ -1926,7 +1898,7 @@ class PDFPageProxy { for (const internalRenderTask of intentState.renderTasks) { internalRenderTask.operatorListChanged(); } - this.#tryCleanup(/* delayed = */ true); + this.#tryCleanup(); } if (intentState.displayReadyCapability) { @@ -2884,13 +2856,6 @@ class WorkerTransport { switch (type) { case "Image": - pageProxy.objs.resolve(id, imageData); - - // Heuristic that will allow us not to store large data. - if (imageData?.dataLen > MAX_IMAGE_SIZE_TO_CACHE) { - pageProxy._maybeCleanupAfterRender = true; - } - break; case "Pattern": pageProxy.objs.resolve(id, imageData); break; diff --git a/src/shared/util.js b/src/shared/util.js index b4f8824b3..13c3b089a 100644 --- a/src/shared/util.js +++ b/src/shared/util.js @@ -28,8 +28,6 @@ const isNodeJS = const IDENTITY_MATRIX = [1, 0, 0, 1, 0, 0]; const FONT_IDENTITY_MATRIX = [0.001, 0, 0, 0.001, 0, 0]; -const MAX_IMAGE_SIZE_TO_CACHE = 10e6; // Ten megabytes. - // Represent the percentage of the height of a single-line field over // the font size. Acrobat seems to use this value. const LINE_FACTOR = 1.35; @@ -1155,7 +1153,6 @@ export { isNodeJS, LINE_DESCENT_FACTOR, LINE_FACTOR, - MAX_IMAGE_SIZE_TO_CACHE, normalizeUnicode, objectFromMap, objectSize,