[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

@@ -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);
 

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,
 };

src/pdf.js

@@ -73,4 +73,3 @@ exports.RenderingCancelledException =
   pdfjsDisplayDOMUtils.RenderingCancelledException;
 exports.getFilenameFromUrl = pdfjsDisplayDOMUtils.getFilenameFromUrl;
 exports.addLinkAttributes = pdfjsDisplayDOMUtils.addLinkAttributes;
-exports.StatTimer = pdfjsDisplayDOMUtils.StatTimer;

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 () {

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);
     }
   }