From 6b1eda3e1238e7a49e34edb37678764096890f40 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 6 Dec 2017 13:51:04 +0100 Subject: [PATCH 1/3] Move `StatTimer` from `src/shared/util.js` to `src/display/dom_utils.js` Since the `StatTimer` is not used in the worker, duplicating this code on both the main and worker sides seem completely unnecessary. --- src/display/api.js | 11 ++++---- src/display/dom_utils.js | 61 ++++++++++++++++++++++++++++++++++++++++ src/pdf.js | 2 +- src/shared/util.js | 61 ---------------------------------------- 4 files changed, 67 insertions(+), 68 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index ec07806ea..b51ce9c62 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -15,15 +15,14 @@ /* globals requirejs, __non_webpack_require__ */ import { - assert, createPromiseCapability, getVerbosityLevel, info, - InvalidPDFException, isArrayBuffer, isSameOrigin, loadJpegStream, - MessageHandler, MissingPDFException, NativeImageDecoding, PageViewport, - PasswordException, StatTimer, stringToBytes, UnexpectedResponseException, - UnknownErrorException, Util, warn + assert, createPromiseCapability, getVerbosityLevel, info, InvalidPDFException, + isArrayBuffer, isSameOrigin, loadJpegStream, MessageHandler, + MissingPDFException, NativeImageDecoding, PageViewport, PasswordException, + stringToBytes, UnexpectedResponseException, UnknownErrorException, Util, warn } from '../shared/util'; import { DOMCanvasFactory, DOMCMapReaderFactory, getDefaultSetting, - RenderingCancelledException + RenderingCancelledException, StatTimer } from './dom_utils'; import { FontFaceObject, FontLoader } from './font_loader'; import { CanvasGraphics } from './canvas'; diff --git a/src/display/dom_utils.js b/src/display/dom_utils.js index 3a683aa84..2caecba9a 100644 --- a/src/display/dom_utils.js +++ b/src/display/dom_utils.js @@ -461,6 +461,66 @@ function isExternalLinkTargetSet() { } } +var StatTimer = (function StatTimerClosure() { + function rpad(str, pad, length) { + while (str.length < length) { + str += pad; + } + return str; + } + function StatTimer() { + this.started = Object.create(null); + this.times = []; + this.enabled = true; + } + StatTimer.prototype = { + time: function StatTimer_time(name) { + if (!this.enabled) { + return; + } + if (name in this.started) { + warn('Timer is already running for ' + name); + } + this.started[name] = Date.now(); + }, + timeEnd: function StatTimer_timeEnd(name) { + if (!this.enabled) { + return; + } + if (!(name in this.started)) { + warn('Timer has not been started for ' + name); + } + this.times.push({ + 'name': name, + 'start': this.started[name], + 'end': Date.now(), + }); + // Remove timer from started so it can be called again. + delete this.started[name]; + }, + toString: function StatTimer_toString() { + var i, ii; + var times = this.times; + var out = ''; + // Find the longest name for padding purposes. + var longest = 0; + for (i = 0, ii = times.length; i < ii; ++i) { + var name = times[i]['name']; + if (name.length > longest) { + longest = name.length; + } + } + for (i = 0, ii = times.length; i < ii; ++i) { + var span = times[i]; + var duration = span.end - span.start; + out += rpad(span['name'], ' ', longest) + ' ' + duration + 'ms\n'; + } + return out; + }, + }; + return StatTimer; +})(); + export { CustomStyle, RenderingCancelledException, @@ -474,4 +534,5 @@ export { DOMCMapReaderFactory, DOMSVGFactory, SimpleXMLParser, + StatTimer, }; diff --git a/src/pdf.js b/src/pdf.js index 57ab13faf..09bdddff4 100644 --- a/src/pdf.js +++ b/src/pdf.js @@ -73,4 +73,4 @@ exports.RenderingCancelledException = pdfjsDisplayDOMUtils.RenderingCancelledException; exports.getFilenameFromUrl = pdfjsDisplayDOMUtils.getFilenameFromUrl; exports.addLinkAttributes = pdfjsDisplayDOMUtils.addLinkAttributes; -exports.StatTimer = pdfjsSharedUtil.StatTimer; +exports.StatTimer = pdfjsDisplayDOMUtils.StatTimer; diff --git a/src/shared/util.js b/src/shared/util.js index aa1b2e07b..0dd597e2c 100644 --- a/src/shared/util.js +++ b/src/shared/util.js @@ -1118,66 +1118,6 @@ function createPromiseCapability() { return capability; } -var StatTimer = (function StatTimerClosure() { - function rpad(str, pad, length) { - while (str.length < length) { - str += pad; - } - return str; - } - function StatTimer() { - this.started = Object.create(null); - this.times = []; - this.enabled = true; - } - StatTimer.prototype = { - time: function StatTimer_time(name) { - if (!this.enabled) { - return; - } - if (name in this.started) { - warn('Timer is already running for ' + name); - } - this.started[name] = Date.now(); - }, - timeEnd: function StatTimer_timeEnd(name) { - if (!this.enabled) { - return; - } - if (!(name in this.started)) { - warn('Timer has not been started for ' + name); - } - this.times.push({ - 'name': name, - 'start': this.started[name], - 'end': Date.now(), - }); - // Remove timer from started so it can be called again. - delete this.started[name]; - }, - toString: function StatTimer_toString() { - var i, ii; - var times = this.times; - var out = ''; - // Find the longest name for padding purposes. - var longest = 0; - for (i = 0, ii = times.length; i < ii; ++i) { - var name = times[i]['name']; - if (name.length > longest) { - longest = name.length; - } - } - for (i = 0, ii = times.length; i < ii; ++i) { - var span = times[i]; - var duration = span.end - span.start; - out += rpad(span['name'], ' ', longest) + ' ' + duration + 'ms\n'; - } - return out; - }, - }; - return StatTimer; -})(); - var createBlob = function createBlob(data, contentType) { if (typeof Blob !== 'undefined') { return new Blob([data], { type: contentType, }); @@ -1668,7 +1608,6 @@ export { PageViewport, PasswordException, PasswordResponses, - StatTimer, StreamType, TextRenderingMode, UnexpectedResponseException, From 50b72dec6e3f0a959927f6ac1fcf69b7c76c7caf Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 6 Dec 2017 13:59:03 +0100 Subject: [PATCH 2/3] Convert `StatTimer` to an ES6 class --- src/display/dom_utils.js | 102 ++++++++++++++++++--------------------- 1 file changed, 47 insertions(+), 55 deletions(-) diff --git a/src/display/dom_utils.js b/src/display/dom_utils.js index 2caecba9a..7a1b8f2d7 100644 --- a/src/display/dom_utils.js +++ b/src/display/dom_utils.js @@ -461,65 +461,57 @@ function isExternalLinkTargetSet() { } } -var StatTimer = (function StatTimerClosure() { - function rpad(str, pad, length) { - while (str.length < length) { - str += pad; - } - return str; - } - function StatTimer() { +class StatTimer { + constructor(enable = true) { this.started = Object.create(null); this.times = []; - this.enabled = true; + this.enabled = !!enable; } - StatTimer.prototype = { - time: function StatTimer_time(name) { - if (!this.enabled) { - return; + + time(name) { + if (!this.enabled) { + return; + } + if (name in this.started) { + warn('Timer is already running for ' + name); + } + this.started[name] = Date.now(); + } + + timeEnd(name) { + if (!this.enabled) { + return; + } + if (!(name in this.started)) { + warn('Timer has not been started for ' + name); + } + this.times.push({ + 'name': name, + 'start': this.started[name], + 'end': Date.now(), + }); + // Remove timer from started so it can be called again. + delete this.started[name]; + } + + toString() { + let times = this.times; + // Find the longest name for padding purposes. + let out = '', longest = 0; + for (let i = 0, ii = times.length; i < ii; ++i) { + let name = times[i]['name']; + if (name.length > longest) { + longest = name.length; } - if (name in this.started) { - warn('Timer is already running for ' + name); - } - this.started[name] = Date.now(); - }, - timeEnd: function StatTimer_timeEnd(name) { - if (!this.enabled) { - return; - } - if (!(name in this.started)) { - warn('Timer has not been started for ' + name); - } - this.times.push({ - 'name': name, - 'start': this.started[name], - 'end': Date.now(), - }); - // Remove timer from started so it can be called again. - delete this.started[name]; - }, - toString: function StatTimer_toString() { - var i, ii; - var times = this.times; - var out = ''; - // Find the longest name for padding purposes. - var longest = 0; - for (i = 0, ii = times.length; i < ii; ++i) { - var name = times[i]['name']; - if (name.length > longest) { - longest = name.length; - } - } - for (i = 0, ii = times.length; i < ii; ++i) { - var span = times[i]; - var duration = span.end - span.start; - out += rpad(span['name'], ' ', longest) + ' ' + duration + 'ms\n'; - } - return out; - }, - }; - return StatTimer; -})(); + } + for (let i = 0, ii = times.length; i < ii; ++i) { + let span = times[i]; + let duration = span.end - span.start; + out += `${span['name'].padEnd(longest)} ${duration}ms\n`; + } + return out; + } +} export { CustomStyle, From 7c5ba9aad532960a7636ec0ba646e6877aeeefaa Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 6 Dec 2017 16:30:04 +0100 Subject: [PATCH 3/3] [api-major] Only create a `StatTimer` for pages when `enableStats == true` (issue 5215) Unless the debugging tools (i.e. `PDFBug`) are enabled, or the `browsertest` is running, the `PDFPageProxy.stats` aren't actually used for anything. Rather than initializing unnecessary `StatTimer` instances, we can simply re-use *one* dummy class (with static methods) for every page. Note that by using a dummy `StatTimer` in this way, rather than letting `PDFPageProxy.stats` be undefined, we don't need to guard *every* single stats collection callsite. Since it wouldn't make much sense to attempt to use `PDFPageProxy.stats` when stat collection is disabled, it was instead changed to a "private" property (i.e. `PDFPageProxy._stats`) and a getter was added for accessing `PDFPageProxy.stats`. This getter will now return `null` when stat collection is disabled, making that case easy to handle. For benchmarking purposes, the test-suite used to re-create the `StatTimer` after loading/rendering each page. However, modifying properties on various API code from the outside in this way seems very error-prone, and is an anti-pattern that we really should avoid at all cost. Hence the `PDFPageProxy.cleanup` method was modified to accept an optional parameter, which will take care of resetting `this.stats` when necessary, and `test/driver.js` was updated accordingly. Finally, a tiny bit more validation was added on the viewer side, to ensure that all the code we're attempting to access is defined when handling `PDFPageProxy` stats. --- src/display/api.js | 30 +++++++++++++++++++++--------- src/display/dom_utils.js | 31 ++++++++++++++++++++++++++++++- src/pdf.js | 1 - test/driver.js | 9 ++++----- web/app.js | 7 ++++--- 5 files changed, 59 insertions(+), 19 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index b51ce9c62..a3fa08ce1 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -21,7 +21,7 @@ import { stringToBytes, UnexpectedResponseException, UnknownErrorException, Util, warn } from '../shared/util'; import { - DOMCanvasFactory, DOMCMapReaderFactory, getDefaultSetting, + DOMCanvasFactory, DOMCMapReaderFactory, DummyStatTimer, getDefaultSetting, RenderingCancelledException, StatTimer } from './dom_utils'; import { FontFaceObject, FontLoader } from './font_loader'; @@ -734,8 +734,8 @@ var PDFPageProxy = (function PDFPageProxyClosure() { this.pageIndex = pageIndex; this.pageInfo = pageInfo; this.transport = transport; - this.stats = new StatTimer(); - this.stats.enabled = getDefaultSetting('enableStats'); + this._stats = (getDefaultSetting('enableStats') ? + new StatTimer() : DummyStatTimer); this.commonObjs = transport.commonObjs; this.objs = new PDFObjects(); this.cleanupAfterRender = false; @@ -811,7 +811,7 @@ var PDFPageProxy = (function PDFPageProxyClosure() { * is resolved when the page finishes rendering. */ render: function PDFPageProxy_render(params) { - var stats = this.stats; + let stats = this._stats; stats.time('Overall'); // If there was a pending destroy cancel it so no cleanup happens during @@ -842,7 +842,7 @@ var PDFPageProxy = (function PDFPageProxyClosure() { lastChunk: false, }; - this.stats.time('Page Request'); + stats.time('Page Request'); this.transport.messageHandler.send('RenderPageRequest', { pageIndex: this.pageNumber - 1, intent: renderingIntent, @@ -1020,17 +1020,19 @@ var PDFPageProxy = (function PDFPageProxyClosure() { /** * Cleans up resources allocated by the page. + * @param {boolean} resetStats - (optional) Reset page stats, if enabled. + * The default value is `false`. */ - cleanup: function PDFPageProxy_cleanup() { + cleanup(resetStats = false) { this.pendingCleanup = true; - this._tryCleanup(); + this._tryCleanup(resetStats); }, /** * For internal use only. Attempts to clean up if rendering is in a state * where that's possible. * @ignore */ - _tryCleanup: function PDFPageProxy_tryCleanup() { + _tryCleanup(resetStats = false) { if (!this.pendingCleanup || Object.keys(this.intentStates).some(function(intent) { var intentState = this.intentStates[intent]; @@ -1045,6 +1047,9 @@ var PDFPageProxy = (function PDFPageProxyClosure() { }, this); this.objs.clear(); this.annotationsPromise = null; + if (resetStats) { + this._stats.reset(); + } this.pendingCleanup = false; }, /** @@ -1086,6 +1091,13 @@ var PDFPageProxy = (function PDFPageProxyClosure() { this._tryCleanup(); } }, + + /** + * @return {Object} Returns page stats, if enabled. + */ + get stats() { + return (this._stats instanceof StatTimer ? this._stats : null); + }, }; return PDFPageProxy; })(); @@ -1704,7 +1716,7 @@ var WorkerTransport = (function WorkerTransportClosure() { } var page = this.pageCache[data.pageIndex]; - page.stats.timeEnd('Page Request'); + page._stats.timeEnd('Page Request'); page._startRenderPage(data.transparency, data.intent); }, this); diff --git a/src/display/dom_utils.js b/src/display/dom_utils.js index 7a1b8f2d7..45ab29c06 100644 --- a/src/display/dom_utils.js +++ b/src/display/dom_utils.js @@ -463,9 +463,13 @@ function isExternalLinkTargetSet() { class StatTimer { constructor(enable = true) { + this.enabled = !!enable; + this.reset(); + } + + reset() { this.started = Object.create(null); this.times = []; - this.enabled = !!enable; } time(name) { @@ -513,6 +517,30 @@ class StatTimer { } } +/** + * Helps avoid having to initialize {StatTimer} instances, e.g. one for every + * page, in cases where the collected stats are not actually being used. + * This (dummy) class can thus, since all its methods are `static`, be directly + * shared between multiple call-sites without the need to be initialized first. + * + * NOTE: This must implement the same interface as {StatTimer}. + */ +class DummyStatTimer { + constructor() { + throw new Error('Cannot initialize DummyStatTimer.'); + } + + static reset() {} + + static time(name) {} + + static timeEnd(name) {} + + static toString() { + return ''; + } +} + export { CustomStyle, RenderingCancelledException, @@ -527,4 +555,5 @@ export { DOMSVGFactory, SimpleXMLParser, StatTimer, + DummyStatTimer, }; diff --git a/src/pdf.js b/src/pdf.js index 09bdddff4..2f8728ae7 100644 --- a/src/pdf.js +++ b/src/pdf.js @@ -73,4 +73,3 @@ exports.RenderingCancelledException = pdfjsDisplayDOMUtils.RenderingCancelledException; exports.getFilenameFromUrl = pdfjsDisplayDOMUtils.getFilenameFromUrl; exports.addLinkAttributes = pdfjsDisplayDOMUtils.addLinkAttributes; -exports.StatTimer = pdfjsDisplayDOMUtils.StatTimer; diff --git a/test/driver.js b/test/driver.js index 6a0ea0183..fa65afe8e 100644 --- a/test/driver.js +++ b/test/driver.js @@ -19,8 +19,6 @@ var WAITING_TIME = 100; // ms var PDF_TO_CSS_UNITS = 96.0 / 72.0; -var StatTimer = pdfjsDistBuildPdf.StatTimer; - /** * @class */ @@ -536,9 +534,10 @@ var Driver = (function DriverClosure() { // eslint-disable-line no-unused-vars if (annotationLayerCanvas) { ctx.drawImage(annotationLayerCanvas, 0, 0); } - page.cleanup(); - task.stats = page.stats; - page.stats = new StatTimer(); + if (page.stats) { // Get the page stats *before* running cleanup. + task.stats = page.stats; + } + page.cleanup(/* resetStats = */ true); self._snapshot(task, error); }); initPromise.then(function () { diff --git a/web/app.js b/web/app.js index f1d9e7271..abdf3a681 100644 --- a/web/app.js +++ b/web/app.js @@ -1668,7 +1668,8 @@ function webViewerPageRendered(evt) { thumbnailView.setImage(pageView); } - if (PDFJS.pdfBug && Stats.enabled && pageView.stats) { + if (PDFJS.pdfBug && typeof Stats !== 'undefined' && Stats.enabled && + pageView.stats) { Stats.add(pageNumber, pageView.stats); } @@ -1957,9 +1958,9 @@ function webViewerPageChanging(evt) { } // we need to update stats - if (PDFJS.pdfBug && Stats.enabled) { + if (PDFJS.pdfBug && typeof Stats !== 'undefined' && Stats.enabled) { let pageView = PDFViewerApplication.pdfViewer.getPageView(page - 1); - if (pageView.stats) { + if (pageView && pageView.stats) { Stats.add(page, pageView.stats); } }