From ae0d9e8c2a629bfe77f5fea409b232377a9dfd36 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 30 Aug 2019 11:35:05 +0200 Subject: [PATCH 1/4] Replace some instances of implicit `function.bind(this)` usage, in `src/display/api.js`, with arrow functions instead --- src/display/api.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index 171946b5c..0521bf5dd 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -1248,17 +1248,17 @@ class PDFPageProxy { */ _tryCleanup(resetStats = false) { if (!this.pendingCleanup || - Object.keys(this.intentStates).some(function(intent) { + Object.keys(this.intentStates).some((intent) => { const intentState = this.intentStates[intent]; return (intentState.renderTasks.length !== 0 || !intentState.operatorList.lastChunk); - }, this)) { + })) { return; } - Object.keys(this.intentStates).forEach(function(intent) { + Object.keys(this.intentStates).forEach((intent) => { delete this.intentStates[intent]; - }, this); + }); this.objs.clear(); this.annotationsPromise = null; if (resetStats && this._stats instanceof StatTimer) { @@ -1443,18 +1443,18 @@ class LoopbackPort { } if (!this._defer) { - this._listeners.forEach(function(listener) { + this._listeners.forEach((listener) => { listener.call(this, { data: obj, }); - }, this); + }); return; } const cloned = new WeakMap(); const e = { data: cloneValue(obj), }; this._deferred.then(() => { - this._listeners.forEach(function(listener) { + this._listeners.forEach((listener) => { listener.call(this, e); - }, this); + }); }); } From 252a3e35fb77de47e6e7202a63a54129556ceb84 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 30 Aug 2019 11:59:04 +0200 Subject: [PATCH 2/4] Reduce the amount of unnecessary function calls and object allocations, in `MessageHandler`, when using Streams With PR 11069 we're now using Streams for OperatorList parsing (in addition to just TextContent parsing), which brings the nice benefit of being able to easily abort parsing on the worker-thread thus saving resources. However, since we're now creating many more `ReadableStream` there appears to be a tiny bit more overhead because of it (giving ~1% slower runtime of `browsertest` on the bots). In this case we're just going to have to accept such a small regression, since the benefits of using Streams clearly outweighs it. What we *can* do here, is to try and make the Streams part of the `MessageHandler` implementation slightly more efficient by e.g. removing unnecessary function calls (which has been helpful in other parts of the code-base). To that end, this patch makes the following changes: - Actually support `transfers` in `MessageHandler.sendWithStream`, since the parameter was being ignored. - Inline the `sendStreamRequest`/`sendStreamResponse` helper functions at their respective call-sites. Obviously this causes some amount of code duplication, however I still think this change seems reasonable since for each call-site: - It avoids making one unnecessary function call. - It avoids allocating one temporary object. - It avoids sending, and thus structure clone, various undefined object properties. - Inline objects in the `MessageHandler.{send, sendWithPromise}` methods. - Finally, directly call `comObj.postMessage` in various methods when `transfers` are *not* present, rather than calling `MessageHandler.postMessage`, to further reduce the amount of function calls. --- src/shared/message_handler.js | 127 +++++++++++++++++++++++----------- 1 file changed, 87 insertions(+), 40 deletions(-) diff --git a/src/shared/message_handler.js b/src/shared/message_handler.js index 29e16b718..961c19be2 100644 --- a/src/shared/message_handler.js +++ b/src/shared/message_handler.js @@ -143,13 +143,12 @@ MessageHandler.prototype = { * @param {Array} [transfers] - Optional list of transfers/ArrayBuffers */ send(actionName, data, transfers) { - var message = { + this.postMessage({ sourceName: this.sourceName, targetName: this.targetName, action: actionName, data, - }; - this.postMessage(message, transfers); + }, transfers); }, /** * Sends a message to the comObj to invoke the action with the supplied data. @@ -161,19 +160,18 @@ MessageHandler.prototype = { */ sendWithPromise(actionName, data, transfers) { var callbackId = this.callbackId++; - var message = { - sourceName: this.sourceName, - targetName: this.targetName, - action: actionName, - data, - callbackId, - }; var capability = createPromiseCapability(); this.callbacksCapabilities[callbackId] = capability; try { - this.postMessage(message, transfers); - } catch (e) { - capability.reject(e); + this.postMessage({ + sourceName: this.sourceName, + targetName: this.targetName, + action: actionName, + callbackId, + data, + }, transfers); + } catch (ex) { + capability.reject(ex); } return capability.promise; }, @@ -191,6 +189,7 @@ MessageHandler.prototype = { let streamId = this.streamId++; let sourceName = this.sourceName; let targetName = this.targetName; + const comObj = this.comObj; return new ReadableStream({ start: (controller) => { @@ -207,7 +206,7 @@ MessageHandler.prototype = { streamId, data, desiredSize: controller.desiredSize, - }); + }, transfers); // Return Promise for Async process, to signal success/failure. return startCapability.promise; }, @@ -215,7 +214,7 @@ MessageHandler.prototype = { pull: (controller) => { let pullCapability = createPromiseCapability(); this.streamControllers[streamId].pullCall = pullCapability; - this.postMessage({ + comObj.postMessage({ sourceName, targetName, stream: 'pull', @@ -231,12 +230,12 @@ MessageHandler.prototype = { let cancelCapability = createPromiseCapability(); this.streamControllers[streamId].cancelCall = cancelCapability; this.streamControllers[streamId].isClosed = true; - this.postMessage({ + comObj.postMessage({ sourceName, targetName, stream: 'cancel', - reason, streamId, + reason, }); // Return Promise to signal success or failure. return cancelCapability.promise; @@ -252,12 +251,7 @@ MessageHandler.prototype = { let sourceName = this.sourceName; let targetName = data.sourceName; let capability = createPromiseCapability(); - - let sendStreamRequest = ({ stream, chunk, transfers, - success, reason, }) => { - this.postMessage({ sourceName, targetName, stream, streamId, - chunk, success, reason, }, transfers); - }; + const comObj = this.comObj; let streamSink = { enqueue(chunk, size = 1, transfers) { @@ -273,7 +267,13 @@ MessageHandler.prototype = { this.sinkCapability = createPromiseCapability(); this.ready = this.sinkCapability.promise; } - sendStreamRequest({ stream: 'enqueue', chunk, transfers, }); + self.postMessage({ + sourceName, + targetName, + stream: 'enqueue', + streamId, + chunk, + }, transfers); }, close() { @@ -281,7 +281,12 @@ MessageHandler.prototype = { return; } this.isCancelled = true; - sendStreamRequest({ stream: 'close', }); + comObj.postMessage({ + sourceName, + targetName, + stream: 'close', + streamId, + }); delete self.streamSinks[streamId]; }, @@ -290,7 +295,13 @@ MessageHandler.prototype = { return; } this.isCancelled = true; - sendStreamRequest({ stream: 'error', reason, }); + comObj.postMessage({ + sourceName, + targetName, + stream: 'error', + streamId, + reason, + }); }, sinkCapability: capability, @@ -305,9 +316,21 @@ MessageHandler.prototype = { streamSink.ready = streamSink.sinkCapability.promise; this.streamSinks[streamId] = streamSink; resolveCall(action[0], [data.data, streamSink], action[1]).then(() => { - sendStreamRequest({ stream: 'start_complete', success: true, }); + comObj.postMessage({ + sourceName, + targetName, + stream: 'start_complete', + streamId, + success: true, + }); }, (reason) => { - sendStreamRequest({ stream: 'start_complete', success: false, reason, }); + comObj.postMessage({ + sourceName, + targetName, + stream: 'start_complete', + streamId, + reason, + }); }); }, @@ -315,11 +338,7 @@ MessageHandler.prototype = { let sourceName = this.sourceName; let targetName = data.sourceName; let streamId = data.streamId; - - let sendStreamResponse = ({ stream, success, reason, }) => { - this.comObj.postMessage({ sourceName, targetName, stream, - success, streamId, reason, }); - }; + const comObj = this.comObj; let deleteStreamController = () => { // Delete the `streamController` only when the start, pull, and cancel @@ -345,7 +364,13 @@ MessageHandler.prototype = { case 'pull': // Ignore any pull after close is called. if (!this.streamSinks[data.streamId]) { - sendStreamResponse({ stream: 'pull_complete', success: true, }); + comObj.postMessage({ + sourceName, + targetName, + stream: 'pull_complete', + streamId, + success: true, + }); break; } // Pull increases the desiredSize property of sink, @@ -358,10 +383,21 @@ MessageHandler.prototype = { // Reset desiredSize property of sink on every pull. this.streamSinks[data.streamId].desiredSize = data.desiredSize; resolveCall(this.streamSinks[data.streamId].onPull).then(() => { - sendStreamResponse({ stream: 'pull_complete', success: true, }); + comObj.postMessage({ + sourceName, + targetName, + stream: 'pull_complete', + streamId, + success: true, + }); }, (reason) => { - sendStreamResponse({ stream: 'pull_complete', - success: false, reason, }); + comObj.postMessage({ + sourceName, + targetName, + stream: 'pull_complete', + streamId, + reason, + }); }); break; case 'enqueue': @@ -398,10 +434,21 @@ MessageHandler.prototype = { } resolveCall(this.streamSinks[data.streamId].onCancel, [wrapReason(data.reason)]).then(() => { - sendStreamResponse({ stream: 'cancel_complete', success: true, }); + comObj.postMessage({ + sourceName, + targetName, + stream: 'cancel_complete', + streamId, + success: true, + }); }, (reason) => { - sendStreamResponse({ stream: 'cancel_complete', - success: false, reason, }); + comObj.postMessage({ + sourceName, + targetName, + stream: 'cancel_complete', + streamId, + reason, + }); }); this.streamSinks[data.streamId].sinkCapability. reject(wrapReason(data.reason)); From 4e6a9b54c792419ff5e3dff9258f7c07d533af1c Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 30 Aug 2019 13:26:39 +0200 Subject: [PATCH 3/4] Change the internal `stream` property, as sent when Streams are used, from a String to a Number Given that the `stream` property is an internal implementation detail, changing its type shouldn't be a problem. By using Numbers instead, we can avoid unnecessary String allocations when creating/processing Streams. --- src/shared/message_handler.js | 52 +++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/src/shared/message_handler.js b/src/shared/message_handler.js index 961c19be2..f5dc2b4f1 100644 --- a/src/shared/message_handler.js +++ b/src/shared/message_handler.js @@ -18,6 +18,18 @@ import { ReadableStream, UnexpectedResponseException, UnknownErrorException } from './util'; +const StreamKind = { + UNKNOWN: 0, + CANCEL: 1, + CANCEL_COMPLETE: 2, + CLOSE: 3, + ENQUEUE: 4, + ERROR: 5, + PULL: 6, + PULL_COMPLETE: 7, + START_COMPLETE: 8, +}; + async function resolveCall(fn, args, thisArg = null) { if (!fn) { return undefined; @@ -217,7 +229,7 @@ MessageHandler.prototype = { comObj.postMessage({ sourceName, targetName, - stream: 'pull', + stream: StreamKind.PULL, streamId, desiredSize: controller.desiredSize, }); @@ -233,7 +245,7 @@ MessageHandler.prototype = { comObj.postMessage({ sourceName, targetName, - stream: 'cancel', + stream: StreamKind.CANCEL, streamId, reason, }); @@ -270,7 +282,7 @@ MessageHandler.prototype = { self.postMessage({ sourceName, targetName, - stream: 'enqueue', + stream: StreamKind.ENQUEUE, streamId, chunk, }, transfers); @@ -284,7 +296,7 @@ MessageHandler.prototype = { comObj.postMessage({ sourceName, targetName, - stream: 'close', + stream: StreamKind.CLOSE, streamId, }); delete self.streamSinks[streamId]; @@ -298,7 +310,7 @@ MessageHandler.prototype = { comObj.postMessage({ sourceName, targetName, - stream: 'error', + stream: StreamKind.ERROR, streamId, reason, }); @@ -319,7 +331,7 @@ MessageHandler.prototype = { comObj.postMessage({ sourceName, targetName, - stream: 'start_complete', + stream: StreamKind.START_COMPLETE, streamId, success: true, }); @@ -327,7 +339,7 @@ MessageHandler.prototype = { comObj.postMessage({ sourceName, targetName, - stream: 'start_complete', + stream: StreamKind.START_COMPLETE, streamId, reason, }); @@ -355,19 +367,19 @@ MessageHandler.prototype = { }; switch (data.stream) { - case 'start_complete': + case StreamKind.START_COMPLETE: resolveOrReject(this.streamControllers[data.streamId].startCall, data); break; - case 'pull_complete': + case StreamKind.PULL_COMPLETE: resolveOrReject(this.streamControllers[data.streamId].pullCall, data); break; - case 'pull': + case StreamKind.PULL: // Ignore any pull after close is called. if (!this.streamSinks[data.streamId]) { comObj.postMessage({ sourceName, targetName, - stream: 'pull_complete', + stream: StreamKind.PULL_COMPLETE, streamId, success: true, }); @@ -386,7 +398,7 @@ MessageHandler.prototype = { comObj.postMessage({ sourceName, targetName, - stream: 'pull_complete', + stream: StreamKind.PULL_COMPLETE, streamId, success: true, }); @@ -394,20 +406,20 @@ MessageHandler.prototype = { comObj.postMessage({ sourceName, targetName, - stream: 'pull_complete', + stream: StreamKind.PULL_COMPLETE, streamId, reason, }); }); break; - case 'enqueue': + case StreamKind.ENQUEUE: assert(this.streamControllers[data.streamId], 'enqueue should have stream controller'); if (!this.streamControllers[data.streamId].isClosed) { this.streamControllers[data.streamId].controller.enqueue(data.chunk); } break; - case 'close': + case StreamKind.CLOSE: assert(this.streamControllers[data.streamId], 'close should have stream controller'); if (this.streamControllers[data.streamId].isClosed) { @@ -417,18 +429,18 @@ MessageHandler.prototype = { this.streamControllers[data.streamId].controller.close(); deleteStreamController(); break; - case 'error': + case StreamKind.ERROR: assert(this.streamControllers[data.streamId], 'error should have stream controller'); this.streamControllers[data.streamId].controller. error(wrapReason(data.reason)); deleteStreamController(); break; - case 'cancel_complete': + case StreamKind.CANCEL_COMPLETE: resolveOrReject(this.streamControllers[data.streamId].cancelCall, data); deleteStreamController(); break; - case 'cancel': + case StreamKind.CANCEL: if (!this.streamSinks[data.streamId]) { break; } @@ -437,7 +449,7 @@ MessageHandler.prototype = { comObj.postMessage({ sourceName, targetName, - stream: 'cancel_complete', + stream: StreamKind.CANCEL_COMPLETE, streamId, success: true, }); @@ -445,7 +457,7 @@ MessageHandler.prototype = { comObj.postMessage({ sourceName, targetName, - stream: 'cancel_complete', + stream: StreamKind.CANCEL_COMPLETE, streamId, reason, }); From f71ea2de0e1064939c7cf7f1683807a037307adc Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 30 Aug 2019 19:22:27 +0200 Subject: [PATCH 4/4] Remove the `makeReasonSerializable` helper function, and use `wrapReason` instead, in `src/shared/message_handler.js` Since `wrapReason` and `makeReasonSerializable` are essentially functionally equivalent it doesn't seem necessary to keep both of them around, especially when `makeReasonSerializable` only has a *single* call-site. --- src/shared/message_handler.js | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/shared/message_handler.js b/src/shared/message_handler.js index f5dc2b4f1..b371a9037 100644 --- a/src/shared/message_handler.js +++ b/src/shared/message_handler.js @@ -48,22 +48,13 @@ function wrapReason(reason) { return new MissingPDFException(reason.message); case 'UnexpectedResponseException': return new UnexpectedResponseException(reason.message, reason.status); - default: + case 'UnknownErrorException': return new UnknownErrorException(reason.message, reason.details); + default: + return new UnknownErrorException(reason.message, reason.toString()); } } -function makeReasonSerializable(reason) { - if (!(reason instanceof Error) || - reason instanceof AbortException || - reason instanceof MissingPDFException || - reason instanceof UnexpectedResponseException || - reason instanceof UnknownErrorException) { - return reason; - } - return new UnknownErrorException(reason.message, reason.toString()); -} - function resolveOrReject(capability, data) { if (data.success) { capability.resolve(); @@ -125,7 +116,7 @@ function MessageHandler(sourceName, targetName, comObj) { targetName, isReply: true, callbackId: data.callbackId, - error: makeReasonSerializable(reason), + error: wrapReason(reason), }); }); } else if (data.streamId) {