From 691be77f65eec60d8b95a980a6dddd638672ec1e Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 16 Nov 2024 11:51:03 +0100 Subject: [PATCH] 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();