From 729e3190ebcf8ad1ce5a1f390d9cefa609055ff5 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 16 Nov 2024 11:50:51 +0100 Subject: [PATCH 1/3] Fix a couple of bugs affecting benchmarking - Ensure that `pdfjsTestingUtils` is available when running benchmarking, since that shouldn't be done in TESTING-mode. - Exclude the `test/stats/results/` folder from linting, since it'll contain *generated* JSON-files. --- .prettierignore | 1 + .stylelintignore | 1 + eslint.config.mjs | 1 + src/pdf.js | 2 +- 4 files changed, 4 insertions(+), 1 deletion(-) diff --git a/.prettierignore b/.prettierignore index e33594bd7..90371a116 100644 --- a/.prettierignore +++ b/.prettierignore @@ -6,6 +6,7 @@ external/bcmaps/ external/builder/fixtures/ external/builder/fixtures_babel/ external/quickjs/ +test/stats/results/ test/tmp/ test/pdfs/ web/locale/ diff --git a/.stylelintignore b/.stylelintignore index e33594bd7..90371a116 100644 --- a/.stylelintignore +++ b/.stylelintignore @@ -6,6 +6,7 @@ external/bcmaps/ external/builder/fixtures/ external/builder/fixtures_babel/ external/quickjs/ +test/stats/results/ test/tmp/ test/pdfs/ web/locale/ diff --git a/eslint.config.mjs b/eslint.config.mjs index e0c2b8b01..2956a79ec 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -35,6 +35,7 @@ export default [ "external/builder/fixtures_babel/", "external/quickjs/", "external/openjpeg/", + "test/stats/results/", "test/tmp/", "test/pdfs/", "web/locale/", diff --git a/src/pdf.js b/src/pdf.js index 2febc4227..ecb6606dd 100644 --- a/src/pdf.js +++ b/src/pdf.js @@ -80,7 +80,7 @@ const pdfjsVersion = const pdfjsBuild = typeof PDFJSDev !== "undefined" ? PDFJSDev.eval("BUNDLE_BUILD") : void 0; -if (typeof PDFJSDev !== "undefined" && PDFJSDev.test("TESTING")) { +if (typeof PDFJSDev !== "undefined" && PDFJSDev.test("TESTING || GENERIC")) { globalThis.pdfjsTestingUtils = { HighlightOutliner, }; From 691be77f65eec60d8b95a980a6dddd638672ec1e Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 16 Nov 2024 11:51:03 +0100 Subject: [PATCH 2/3] Convert the `Dict`-implementation to use a `Map` internally With all the recent work happening under https://bugzilla.mozilla.org/show_bug.cgi?id=1851662, the performance of `Map` is already good enough that I believe that we should now be able to utilize it in the `Dict`-class without problem. This patch was tested in Firefox Nightly, specifically build https://hg.mozilla.org/mozilla-central/rev/6c508a387477e3b72db913a9e1761e9a433d06a2, with the following manifest file: ``` [ { "id": "tracemonkey-eq", "file": "pdfs/tracemonkey.pdf", "md5": "9a192d8b1a7dc652a19835f6f08098bd", "rounds": 100, "type": "eq" }, { "id": "issue2618", "file": "pdfs/issue2618.pdf", "md5": "2c554a99a52288ca1a44a422eeafb8fb", "rounds": 100, "type": "eq" } ] ``` which gave the following results, indicating no significant regression, when comparing this patch against the `master` branch: - Overall ``` -- Grouped By browser, pdf, stat -- browser | pdf | stat | Count | Baseline(ms) | Current(ms) | +/- | % | Result(P<.05) ------- | -------------- | ------------ | ----- | ------------ | ----------- | --- | ----- | ------------- firefox | issue2618 | Overall | 100 | 678 | 678 | 0 | 0.04 | firefox | issue2618 | Page Request | 100 | 1 | 1 | 0 | -3.88 | firefox | issue2618 | Rendering | 100 | 677 | 677 | 0 | 0.05 | firefox | tracemonkey-eq | Overall | 1400 | 35 | 36 | 0 | 0.96 | firefox | tracemonkey-eq | Page Request | 1400 | 1 | 1 | 0 | -8.08 | firefox | tracemonkey-eq | Rendering | 1400 | 34 | 35 | 0 | 1.26 | ``` - Page-specific ``` -- Grouped By browser, pdf, page, stat -- browser | pdf | page | stat | Count | Baseline(ms) | Current(ms) | +/- | % | Result(P<.05) ------- | -------------- | ---- | ------------ | ----- | ------------ | ----------- | --- | ------ | ------------- firefox | issue2618 | 0 | Overall | 100 | 678 | 678 | 0 | 0.04 | firefox | issue2618 | 0 | Page Request | 100 | 1 | 1 | 0 | -3.88 | firefox | issue2618 | 0 | Rendering | 100 | 677 | 677 | 0 | 0.05 | firefox | tracemonkey-eq | 0 | Overall | 100 | 23 | 24 | 0 | 1.24 | firefox | tracemonkey-eq | 0 | Page Request | 100 | 1 | 1 | 0 | 19.77 | firefox | tracemonkey-eq | 0 | Rendering | 100 | 23 | 23 | 0 | 0.40 | firefox | tracemonkey-eq | 1 | Overall | 100 | 32 | 32 | -1 | -1.89 | firefox | tracemonkey-eq | 1 | Page Request | 100 | 1 | 1 | 0 | -28.13 | firefox | tracemonkey-eq | 1 | Rendering | 100 | 31 | 31 | 0 | -0.77 | firefox | tracemonkey-eq | 2 | Overall | 100 | 17 | 18 | 1 | 4.60 | firefox | tracemonkey-eq | 2 | Page Request | 100 | 1 | 1 | 0 | 23.53 | slower firefox | tracemonkey-eq | 2 | Rendering | 100 | 17 | 17 | 1 | 3.71 | firefox | tracemonkey-eq | 3 | Overall | 100 | 23 | 24 | 0 | 1.71 | firefox | tracemonkey-eq | 3 | Page Request | 100 | 1 | 1 | 0 | 7.79 | firefox | tracemonkey-eq | 3 | Rendering | 100 | 23 | 23 | 0 | 1.55 | firefox | tracemonkey-eq | 4 | Overall | 100 | 31 | 31 | 1 | 2.49 | firefox | tracemonkey-eq | 4 | Page Request | 100 | 1 | 1 | 0 | 48.96 | firefox | tracemonkey-eq | 4 | Rendering | 100 | 30 | 30 | 0 | 1.05 | firefox | tracemonkey-eq | 5 | Overall | 100 | 31 | 30 | -1 | -2.42 | firefox | tracemonkey-eq | 5 | Page Request | 100 | 2 | 1 | -1 | -49.33 | firefox | tracemonkey-eq | 5 | Rendering | 100 | 29 | 29 | 0 | -0.03 | firefox | tracemonkey-eq | 6 | Overall | 100 | 27 | 27 | 0 | 1.81 | firefox | tracemonkey-eq | 6 | Page Request | 100 | 1 | 1 | 0 | 4.94 | firefox | tracemonkey-eq | 6 | Rendering | 100 | 26 | 27 | 0 | 1.68 | firefox | tracemonkey-eq | 7 | Overall | 100 | 26 | 26 | 1 | 3.13 | firefox | tracemonkey-eq | 7 | Page Request | 100 | 1 | 1 | 0 | 6.98 | firefox | tracemonkey-eq | 7 | Rendering | 100 | 25 | 25 | 1 | 2.92 | firefox | tracemonkey-eq | 8 | Overall | 100 | 25 | 26 | 1 | 5.16 | firefox | tracemonkey-eq | 8 | Page Request | 100 | 1 | 1 | -1 | -41.84 | firefox | tracemonkey-eq | 8 | Rendering | 100 | 23 | 25 | 2 | 8.19 | firefox | tracemonkey-eq | 9 | Overall | 100 | 33 | 33 | 0 | 0.03 | firefox | tracemonkey-eq | 9 | Page Request | 100 | 1 | 1 | 0 | 0.79 | firefox | tracemonkey-eq | 9 | Rendering | 100 | 32 | 32 | 0 | -0.10 | firefox | tracemonkey-eq | 10 | Overall | 100 | 144 | 144 | 1 | 0.52 | firefox | tracemonkey-eq | 10 | Page Request | 100 | 2 | 1 | -1 | -43.52 | firefox | tracemonkey-eq | 10 | Rendering | 100 | 141 | 143 | 2 | 1.18 | firefox | tracemonkey-eq | 11 | Overall | 100 | 24 | 25 | 1 | 2.51 | firefox | tracemonkey-eq | 11 | Page Request | 100 | 1 | 1 | 0 | -4.71 | firefox | tracemonkey-eq | 11 | Rendering | 100 | 23 | 24 | 1 | 2.78 | firefox | tracemonkey-eq | 12 | Overall | 100 | 40 | 39 | -1 | -1.67 | firefox | tracemonkey-eq | 12 | Page Request | 100 | 1 | 1 | 0 | 14.71 | firefox | tracemonkey-eq | 12 | Rendering | 100 | 39 | 38 | -1 | -1.98 | firefox | tracemonkey-eq | 13 | Overall | 100 | 19 | 20 | 1 | 3.09 | firefox | tracemonkey-eq | 13 | Page Request | 100 | 1 | 1 | 0 | 24.79 | firefox | tracemonkey-eq | 13 | Rendering | 100 | 18 | 19 | 0 | 1.70 | ``` --- src/core/primitives.js | 53 +++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/src/core/primitives.js b/src/core/primitives.js index 0ee534145..bf556b538 100644 --- a/src/core/primitives.js +++ b/src/core/primitives.js @@ -69,7 +69,7 @@ const nonSerializable = function nonSerializableClosure() { class Dict { constructor(xref = null) { // Map should only be used internally, use functions below to access. - this._map = Object.create(null); + this._map = new Map(); this.xref = xref; this.objId = null; this.suppressEncryption = false; @@ -81,12 +81,12 @@ class Dict { } get size() { - return Object.keys(this._map).length; + return this._map.size; } // Automatically dereferences Ref objects. get(key1, key2, key3) { - let value = this._map[key1]; + let value = this._map.get(key1); if (value === undefined && key2 !== undefined) { if ( (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) && @@ -94,7 +94,7 @@ class Dict { ) { unreachable("Dict.get: Expected keys to be ordered by length."); } - value = this._map[key2]; + value = this._map.get(key2); if (value === undefined && key3 !== undefined) { if ( (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) && @@ -102,7 +102,7 @@ class Dict { ) { unreachable("Dict.get: Expected keys to be ordered by length."); } - value = this._map[key3]; + value = this._map.get(key3); } } if (value instanceof Ref && this.xref) { @@ -113,7 +113,7 @@ class Dict { // Same as get(), but returns a promise and uses fetchIfRefAsync(). async getAsync(key1, key2, key3) { - let value = this._map[key1]; + let value = this._map.get(key1); if (value === undefined && key2 !== undefined) { if ( (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) && @@ -121,7 +121,7 @@ class Dict { ) { unreachable("Dict.getAsync: Expected keys to be ordered by length."); } - value = this._map[key2]; + value = this._map.get(key2); if (value === undefined && key3 !== undefined) { if ( (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) && @@ -129,7 +129,7 @@ class Dict { ) { unreachable("Dict.getAsync: Expected keys to be ordered by length."); } - value = this._map[key3]; + value = this._map.get(key3); } } if (value instanceof Ref && this.xref) { @@ -140,7 +140,7 @@ class Dict { // Same as get(), but dereferences all elements if the result is an Array. getArray(key1, key2, key3) { - let value = this._map[key1]; + let value = this._map.get(key1); if (value === undefined && key2 !== undefined) { if ( (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) && @@ -148,7 +148,7 @@ class Dict { ) { unreachable("Dict.getArray: Expected keys to be ordered by length."); } - value = this._map[key2]; + value = this._map.get(key2); if (value === undefined && key3 !== undefined) { if ( (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) && @@ -156,7 +156,7 @@ class Dict { ) { unreachable("Dict.getArray: Expected keys to be ordered by length."); } - value = this._map[key3]; + value = this._map.get(key3); } } if (value instanceof Ref && this.xref) { @@ -176,16 +176,16 @@ class Dict { // No dereferencing. getRaw(key) { - return this._map[key]; + return this._map.get(key); } getKeys() { - return Object.keys(this._map); + return [...this._map.keys()]; } // No dereferencing. getRawValues() { - return Object.values(this._map); + return [...this._map.values()]; } set(key, value) { @@ -196,16 +196,21 @@ class Dict { unreachable('Dict.set: The "value" cannot be undefined.'); } } - this._map[key] = value; + this._map.set(key, value); } has(key) { - return this._map[key] !== undefined; + return this._map.has(key); } forEach(callback) { - for (const key in this._map) { - callback(key, this.get(key)); + for (const [key, value] of this._map) { + callback( + key, + value instanceof Ref && this.xref + ? this.xref.fetch(value, this.suppressEncryption) + : value + ); } } @@ -226,7 +231,7 @@ class Dict { if (!(dict instanceof Dict)) { continue; } - for (const [key, value] of Object.entries(dict._map)) { + for (const [key, value] of dict._map) { let property = properties.get(key); if (property === undefined) { property = []; @@ -242,20 +247,20 @@ class Dict { } for (const [name, values] of properties) { if (values.length === 1 || !(values[0] instanceof Dict)) { - mergedDict._map[name] = values[0]; + mergedDict._map.set(name, values[0]); continue; } const subDict = new Dict(xref); for (const dict of values) { - for (const [key, value] of Object.entries(dict._map)) { - if (subDict._map[key] === undefined) { - subDict._map[key] = value; + for (const [key, value] of dict._map) { + if (!subDict._map.has(key)) { + subDict._map.set(key, value); } } } if (subDict.size > 0) { - mergedDict._map[name] = subDict; + mergedDict._map.set(name, subDict); } } properties.clear(); From 2c0cc48d1ba0b02cff317acaeb034b015cdf9ce4 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 16 Nov 2024 11:51:09 +0100 Subject: [PATCH 3/3] Replace the `forEach` method in `Dict` with "proper" iteration support --- src/core/catalog.js | 4 ++-- src/core/document.js | 7 +------ src/core/primitives.js | 8 ++++---- src/core/struct_tree.js | 9 ++++----- src/core/worker.js | 4 ++-- test/unit/primitives_spec.js | 17 ++++++----------- 6 files changed, 19 insertions(+), 30 deletions(-) diff --git a/src/core/catalog.js b/src/core/catalog.js index 8f09309b9..00524c32e 100644 --- a/src/core/catalog.js +++ b/src/core/catalog.js @@ -720,12 +720,12 @@ class Catalog { } } } else if (obj instanceof Dict) { - obj.forEach(function (key, value) { + for (const [key, value] of obj) { const dest = fetchDest(value); if (dest) { dests[key] = dest; } - }); + } } return shadow(this, "destinations", dests); } diff --git a/src/core/document.js b/src/core/document.js index 3c2599233..1744066d1 100644 --- a/src/core/document.js +++ b/src/core/document.js @@ -1282,13 +1282,8 @@ class PDFDocument { }, }; - const fonts = new Map(); - fontRes.forEach((fontName, font) => { - fonts.set(fontName, font); - }); const promises = []; - - for (const [fontName, font] of fonts) { + for (const [fontName, font] of fontRes) { const descriptor = font.get("FontDescriptor"); if (!(descriptor instanceof Dict)) { continue; diff --git a/src/core/primitives.js b/src/core/primitives.js index bf556b538..c48a34eb7 100644 --- a/src/core/primitives.js +++ b/src/core/primitives.js @@ -203,14 +203,14 @@ class Dict { return this._map.has(key); } - forEach(callback) { + *[Symbol.iterator]() { for (const [key, value] of this._map) { - callback( + yield [ key, value instanceof Ref && this.xref ? this.xref.fetch(value, this.suppressEncryption) - : value - ); + : value, + ]; } } diff --git a/src/core/struct_tree.js b/src/core/struct_tree.js index 7f5c7b45e..fa27a14c0 100644 --- a/src/core/struct_tree.js +++ b/src/core/struct_tree.js @@ -62,12 +62,11 @@ class StructTreeRoot { if (!(roleMapDict instanceof Dict)) { return; } - roleMapDict.forEach((key, value) => { - if (!(value instanceof Name)) { - return; + for (const [key, value] of roleMapDict) { + if (value instanceof Name) { + this.roleMap.set(key, value.name); } - this.roleMap.set(key, value.name); - }); + } } static async canCreateStructureTree({ diff --git a/src/core/worker.js b/src/core/worker.js index cba6cb0ce..e285d6491 100644 --- a/src/core/worker.js +++ b/src/core/worker.js @@ -688,11 +688,11 @@ class WorkerMessageHandler { const infoObj = Object.create(null); const xrefInfo = xref.trailer.get("Info") || null; if (xrefInfo instanceof Dict) { - xrefInfo.forEach((key, value) => { + for (const [key, value] of xrefInfo) { if (typeof value === "string") { infoObj[key] = stringToPDFString(value); } - }); + } } newXrefInfo = { diff --git a/test/unit/primitives_spec.js b/test/unit/primitives_spec.js index caefdf025..00c491004 100644 --- a/test/unit/primitives_spec.js +++ b/test/unit/primitives_spec.js @@ -221,17 +221,12 @@ describe("primitives", function () { expect(values[2]).toEqual(testFontFile); }); - it("should callback for each stored key", function () { - const callbackSpy = jasmine.createSpy("spy on callback in dictionary"); - - dictWithManyKeys.forEach(callbackSpy); - - expect(callbackSpy).toHaveBeenCalled(); - const callbackSpyCalls = callbackSpy.calls; - expect(callbackSpyCalls.argsFor(0)).toEqual(["FontFile", testFontFile]); - expect(callbackSpyCalls.argsFor(1)).toEqual(["FontFile2", testFontFile2]); - expect(callbackSpyCalls.argsFor(2)).toEqual(["FontFile3", testFontFile3]); - expect(callbackSpyCalls.count()).toEqual(3); + it("should iterate through each stored key", function () { + expect([...dictWithManyKeys]).toEqual([ + ["FontFile", testFontFile], + ["FontFile2", testFontFile2], + ["FontFile3", testFontFile3], + ]); }); it("should handle keys pointing to indirect objects, both sync and async", async function () {