From f7ec74c4743d3fb3a709155702b44b469b1d57cb Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 6 Feb 2025 09:09:15 +0100 Subject: [PATCH 1/2] Add a temporary function-cache in `AlternateCS.prototype.getRgbBuffer` This supplements, rather than replaces, the existing caching in `PDFFunction.constructPostScript` since that one still makes sense given that functions are cached on the page-level. Using an additional cache helps improve performance because: - There are many fewer function calls, and overall less parsing this way. - For the common case of `Uint8Array`-data we're able to use integer cache-keys, which is a lot faster than string concatenation. This significantly improves performance of the `pr5134` test-case with `isEvalSupported = false` set, and testing locally in the viewer: - With the `master` branch and `isEvalSupported = true`, page 2 renders in approx. 340 milliseconds. - With the `master` branch and `isEvalSupported = false`, page 2 renders in approx. 1200 milliseconds. - With this patch and `isEvalSupported = false`, page 2 renders in approx. 380 milliseconds. While this is obviously still slower, the difference is now small enough that it shouldn't be too much of an issue in practice (compare with PR 18070) and the `pr5134` test-case is an especially "bad" one w.r.t. its PostScript function use. --- src/core/colorspace.js | 40 ++++++++++++++++++++++++++++++--------- src/core/function.js | 3 +-- test/pdfs/pr5134.pdf.link | 1 + test/test_manifest.json | 9 +++++++++ 4 files changed, 42 insertions(+), 11 deletions(-) create mode 100644 test/pdfs/pr5134.pdf.link diff --git a/src/core/colorspace.js b/src/core/colorspace.js index 287b6cb0b..e06019043 100644 --- a/src/core/colorspace.js +++ b/src/core/colorspace.js @@ -379,24 +379,46 @@ class AlternateCS extends ColorSpace { : new Uint8ClampedArray(baseNumComps * count); const numComps = this.numComps; + const tintCache = new Map(); + // Use an integer cache-key when possible, since that's a lot faster than + // string concatenation. + const int32Key = + numComps <= 4 && + (src instanceof Uint8Array || src instanceof Uint8ClampedArray); + const scaled = new Float32Array(numComps); const tinted = new Float32Array(baseNumComps); - let i, j; + let i, j, key, val; for (i = 0; i < count; i++) { + key = int32Key ? 0 : ""; + for (j = 0; j < numComps; j++) { - scaled[j] = src[srcOffset++] * scale; + val = src[srcOffset++]; + scaled[j] = val * scale; + + key += int32Key ? val << (8 * j) : val + "_"; } - tintFn(scaled, 0, tinted, 0); - if (usesZeroToOneRange) { - for (j = 0; j < baseNumComps; j++) { - baseBuf[pos++] = tinted[j] * 255; - } - } else { - base.getRgbItem(tinted, 0, baseBuf, pos); + + const cached = tintCache.get(key); + if (cached) { + baseBuf.set(cached, pos); pos += baseNumComps; + } else { + tintFn(scaled, 0, tinted, 0); + + if (usesZeroToOneRange) { + for (j = 0; j < baseNumComps; j++) { + baseBuf[pos++] = tinted[j] * 255; + } + } else { + base.getRgbItem(tinted, 0, baseBuf, pos); + pos += baseNumComps; + } + tintCache.set(key, baseBuf.slice(pos - baseNumComps, pos)); } } + tintCache.clear(); if (!isPassthrough) { base.getRgbBuffer(baseBuf, 0, count, dest, destOffset, 8, alpha01); diff --git a/src/core/function.js b/src/core/function.js index f426f15bf..702e36b3e 100644 --- a/src/core/function.js +++ b/src/core/function.js @@ -392,12 +392,11 @@ class PDFFunction { // seen in our tests. const MAX_CACHE_SIZE = 2048 * 4; let cache_available = MAX_CACHE_SIZE; - const tmpBuf = new Float32Array(numInputs); + const input = new Float32Array(numInputs); return function constructPostScriptFn(src, srcOffset, dest, destOffset) { let i, value; let key = ""; - const input = tmpBuf; for (i = 0; i < numInputs; i++) { value = src[srcOffset + i]; input[i] = value; diff --git a/test/pdfs/pr5134.pdf.link b/test/pdfs/pr5134.pdf.link new file mode 100644 index 000000000..721dc2fdc --- /dev/null +++ b/test/pdfs/pr5134.pdf.link @@ -0,0 +1 @@ +https://web.archive.org/web/20140211020222/http://www.coachusa.com/CoachUsaAssets/files/97/route45.pdf diff --git a/test/test_manifest.json b/test/test_manifest.json index bfa914535..394c60a6a 100644 --- a/test/test_manifest.json +++ b/test/test_manifest.json @@ -2254,6 +2254,15 @@ "lastPage": 1, "type": "eq" }, + { + "id": "pr5134", + "file": "pdfs/pr5134.pdf", + "md5": "6a701a163472e071a2519348b55cbac1", + "rounds": 1, + "link": true, + "firstPage": 2, + "type": "eq" + }, { "id": "issue5599", "file": "pdfs/issue5599.pdf", From 602824ba0f7addc08c78bb7c0d26baed162d0b11 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 6 Feb 2025 09:09:29 +0100 Subject: [PATCH 2/2] [api-minor] Disable `eval` support by default The idea behind this patch is to see if disabling of `eval` leads to any reports about bad performance, since the previous patch should have improved things a fair bit. --- src/display/api.js | 4 ++-- web/app_options.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index 815e7ee23..cec2f2fdb 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -159,7 +159,7 @@ const RENDERING_CANCELLED_TIMEOUT = 100; // ms * Use -1 for no limit, which is also the default value. * @property {boolean} [isEvalSupported] - Determines if we can evaluate strings * as JavaScript. Primarily used to improve performance of PDF functions. - * The default value is `true`. + * The default value is `false`. * @property {boolean} [isOffscreenCanvasSupported] - Determines if we can use * `OffscreenCanvas` in the worker. Primarily used to improve performance of * image conversion/rendering. @@ -289,7 +289,7 @@ function getDocument(src = {}) { Number.isInteger(src.maxImageSize) && src.maxImageSize > -1 ? src.maxImageSize : -1; - const isEvalSupported = src.isEvalSupported !== false; + const isEvalSupported = src.isEvalSupported === true; const isOffscreenCanvasSupported = typeof src.isOffscreenCanvasSupported === "boolean" ? src.isOffscreenCanvasSupported diff --git a/web/app_options.js b/web/app_options.js index a21f088b9..43ed2f430 100644 --- a/web/app_options.js +++ b/web/app_options.js @@ -419,7 +419,7 @@ const defaultOptions = { }, isEvalSupported: { /** @type {boolean} */ - value: true, + value: false, kind: OptionKind.API, }, isOffscreenCanvasSupported: {