Skip to content

Commit

Permalink
Remove the RefSet primitive
Browse files Browse the repository at this point in the history
It's only used in `src/core/obj.js` in a handful of places where
nowadays a regular JavaScript set works too (the `RefSet` primitive
predates the usage of ES6). Not only are built-in data structures
often more efficient/optimized, it also avoids having to maintain a
custom data structure.

Previously, each call to a `RefSet` method would cause `toString` to be
called on the given `Ref`. As an additional bonus, using a regular
JavaScript set allows us to limit the number of times `toString` is
called now, simply by calling it once for `has` and `add` calls, for
example. Therefore, this is also more efficient since it avoids the
creation of intermediate strings.
  • Loading branch information
timvandermeij committed Jun 7, 2020
1 parent c97200f commit 84796c4
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 65 deletions.
48 changes: 28 additions & 20 deletions src/core/obj.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ import {
isRefsEqual,
isStream,
Ref,
RefSet,
RefSetCache,
} from "./primitives.js";
import { Lexer, Parser } from "./parser.js";
Expand Down Expand Up @@ -148,8 +147,8 @@ class Catalog {
const root = { items: [] };
const queue = [{ obj, parent: root }];
// To avoid recursion, keep track of the already processed items.
const processed = new RefSet();
processed.put(obj);
const processed = new Set();
processed.add(obj.toString());
const xref = this.xref,
blackColor = new Uint8ClampedArray(3);

Expand Down Expand Up @@ -199,14 +198,20 @@ class Catalog {

i.parent.items.push(outlineItem);
obj = outlineDict.getRaw("First");
if (isRef(obj) && !processed.has(obj)) {
queue.push({ obj, parent: outlineItem });
processed.put(obj);
if (isRef(obj)) {
const objString = obj.toString();
if (!processed.has(objString)) {
queue.push({ obj, parent: outlineItem });
processed.add(objString);
}
}
obj = outlineDict.getRaw("Next");
if (isRef(obj) && !processed.has(obj)) {
queue.push({ obj, parent: i.parent });
processed.put(obj);
if (isRef(obj)) {
const objString = obj.toString();
if (!processed.has(objString)) {
queue.push({ obj, parent: i.parent });
processed.add(objString);
}
}
}
return root.items.length > 0 ? root.items : null;
Expand Down Expand Up @@ -738,7 +743,7 @@ class Catalog {
getPageDict(pageIndex) {
const capability = createPromiseCapability();
const nodesToVisit = [this.catDict.getRaw("Pages")];
const visitedNodes = new RefSet();
const visitedNodes = new Set();
const xref = this.xref,
pageKidsCountCache = this.pageKidsCountCache;
let count,
Expand All @@ -756,13 +761,14 @@ class Catalog {
continue;
}
// Prevent circular references in the /Pages tree.
if (visitedNodes.has(currentNode)) {
const currentNodeStr = currentNode.toString();
if (visitedNodes.has(currentNodeStr)) {
capability.reject(
new FormatError("Pages tree contains circular reference.")
);
return;
}
visitedNodes.put(currentNode);
visitedNodes.add(currentNodeStr);

xref.fetchAsync(currentNode).then(function (obj) {
if (isDict(obj, "Page") || (isDict(obj) && !obj.has("Kids"))) {
Expand Down Expand Up @@ -1962,8 +1968,8 @@ class NameOrNumberTree {
}
const xref = this.xref;
// Reading Name/Number tree.
const processed = new RefSet();
processed.put(this.root);
const processed = new Set();
processed.add(this.root.toString());
const queue = [this.root];
while (queue.length > 0) {
const obj = xref.fetchIfRef(queue.shift());
Expand All @@ -1974,11 +1980,12 @@ class NameOrNumberTree {
const kids = obj.get("Kids");
for (let i = 0, ii = kids.length; i < ii; i++) {
const kid = kids[i];
if (processed.has(kid)) {
const kidStr = kid.toString();
if (processed.has(kidStr)) {
throw new FormatError(`Duplicate entry in "${this._type}" tree.`);
}
queue.push(kid);
processed.put(kid);
processed.add(kidStr);
}
continue;
}
Expand Down Expand Up @@ -2244,7 +2251,7 @@ const ObjectLoader = (function () {
}

const { keys, dict } = this;
this.refSet = new RefSet();
this.refSet = new Set();
// Setup the initial nodes to visit.
const nodesToVisit = [];
for (let i = 0, ii = keys.length; i < ii; i++) {
Expand All @@ -2267,11 +2274,12 @@ const ObjectLoader = (function () {
// Only references or chunked streams can cause missing data exceptions.
if (currentNode instanceof Ref) {
// Skip nodes that have already been visited.
if (this.refSet.has(currentNode)) {
const currentNodeStr = currentNode.toString();
if (this.refSet.has(currentNodeStr)) {
continue;
}
try {
this.refSet.put(currentNode);
this.refSet.add(currentNodeStr);
currentNode = this.xref.fetch(currentNode);
} catch (ex) {
if (!(ex instanceof MissingDataException)) {
Expand Down Expand Up @@ -2307,7 +2315,7 @@ const ObjectLoader = (function () {
// Remove any reference nodes from the current `RefSet` so they
// aren't skipped when we revist them.
if (node instanceof Ref) {
this.refSet.remove(node);
this.refSet.delete(node.toString());
}
}
return this._walk(nodesToRevisit);
Expand Down
26 changes: 0 additions & 26 deletions src/core/primitives.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,31 +219,6 @@ var Ref = (function RefClosure() {
return Ref;
})();

// The reference is identified by number and generation.
// This structure stores only one instance of the reference.
var RefSet = (function RefSetClosure() {
// eslint-disable-next-line no-shadow
function RefSet() {
this.dict = Object.create(null);
}

RefSet.prototype = {
has: function RefSet_has(ref) {
return ref.toString() in this.dict;
},

put: function RefSet_put(ref) {
this.dict[ref.toString()] = true;
},

remove: function RefSet_remove(ref) {
delete this.dict[ref.toString()];
},
};

return RefSet;
})();

var RefSetCache = (function RefSetCacheClosure() {
// eslint-disable-next-line no-shadow
function RefSetCache() {
Expand Down Expand Up @@ -337,7 +312,6 @@ export {
Dict,
Name,
Ref,
RefSet,
RefSetCache,
isEOF,
isCmd,
Expand Down
19 changes: 0 additions & 19 deletions test/unit/primitives_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
isRefsEqual,
Name,
Ref,
RefSet,
} from "../../src/core/primitives.js";
import { XRefMock } from "./test_utils.js";

Expand Down Expand Up @@ -285,24 +284,6 @@ describe("primitives", function () {
});
});

describe("RefSet", function () {
it("should have a stored value", function () {
var ref = Ref.get(4, 2);
var refset = new RefSet();
refset.put(ref);
expect(refset.has(ref)).toBeTruthy();
});
it("should not have an unknown value", function () {
var ref = Ref.get(4, 2);
var refset = new RefSet();
expect(refset.has(ref)).toBeFalsy();

refset.put(ref);
var anotherRef = Ref.get(2, 4);
expect(refset.has(anotherRef)).toBeFalsy();
});
});

describe("isName", function () {
it("handles non-names", function () {
var nonName = {};
Expand Down

0 comments on commit 84796c4

Please sign in to comment.