From 7c5ba9aad532960a7636ec0ba646e6877aeeefaa Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 6 Dec 2017 16:30:04 +0100 Subject: [PATCH] [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); } }