perf: Use WeakMap for dom data (#6103)

videojs · Aug 1, 2019 · 8610f99 · 8610f99
1 parent 1d2b206
commit 8610f99
Show file tree

Hide file tree

Showing 7 changed files with 30 additions and 208 deletions.
diff --git a/src/js/component.js b/src/js/component.js
@@ -7,7 +7,7 @@ import window from 'global/window';
 import evented from './mixins/evented';
 import stateful from './mixins/stateful';
 import * as Dom from './utils/dom.js';
-import * as DomData from './utils/dom-data';
+import DomData from './utils/dom-data';
 import * as Fn from './utils/fn.js';
 import * as Guid from './utils/guid.js';
 import toTitleCase from './utils/to-title-case.js';
@@ -153,7 +153,9 @@ class Component {
         this.el_.parentNode.removeChild(this.el_);
       }
 
-      DomData.removeData(this.el_);
+      if (DomData.has(this.el_)) {
+        DomData.delete(this.el_);
+      }
       this.el_ = null;
     }
 

diff --git a/src/js/utils/dom-data.js b/src/js/utils/dom-data.js
@@ -2,8 +2,6 @@
  * @file dom-data.js
  * @module dom-data
  */
-import * as Guid from './guid.js';
-import window from 'global/window';
 
 /**
  * Element Data Store.
@@ -15,85 +13,4 @@ import window from 'global/window';
  * @type {Object}
  * @private
  */
-export const elData = {};
-
-/*
- * Unique attribute name to store an element's guid in
- *
- * @type {String}
- * @constant
- * @private
- */
-const elIdAttr = 'vdata' + Math.floor(window.performance && window.performance.now() || Date.now());
-
-/**
- * Returns the cache object where data for an element is stored
- *
- * @param {Element} el
- *        Element to store data for.
- *
- * @return {Object}
- *         The cache object for that el that was passed in.
- */
-export function getData(el) {
-  let id = el[elIdAttr];
-
-  if (!id) {
-    id = el[elIdAttr] = Guid.newGUID();
-  }
-
-  if (!elData[id]) {
-    elData[id] = {};
-  }
-
-  return elData[id];
-}
-
-/**
- * Returns whether or not an element has cached data
- *
- * @param {Element} el
- *        Check if this element has cached data.
- *
- * @return {boolean}
- *         - True if the DOM element has cached data.
- *         - False otherwise.
- */
-export function hasData(el) {
-  const id = el[elIdAttr];
-
-  if (!id) {
-    return false;
-  }
-
-  return !!Object.getOwnPropertyNames(elData[id]).length;
-}
-
-/**
- * Delete data for the element from the cache and the guid attr from getElementById
- *
- * @param {Element} el
- *        Remove cached data for this element.
- */
-export function removeData(el) {
-  const id = el[elIdAttr];
-
-  if (!id) {
-    return;
-  }
-
-  // Remove all stored data
-  delete elData[id];
-
-  // Remove the elIdAttr property from the DOM node
-  try {
-    delete el[elIdAttr];
-  } catch (e) {
-    if (el.removeAttribute) {
-      el.removeAttribute(elIdAttr);
-    } else {
-      // IE doesn't appear to support removeAttribute on the document element
-      el[elIdAttr] = null;
-    }
-  }
-}
+export default new WeakMap();
diff --git a/src/js/utils/events.js b/src/js/utils/events.js
@@ -7,7 +7,7 @@
  * @file events.js
  * @module events
  */
-import * as DomData from './dom-data';
+import DomData from './dom-data';
 import * as Guid from './guid.js';
 import log from './log.js';
 import window from 'global/window';
@@ -23,7 +23,10 @@ import document from 'global/document';
  *        Type of event to clean up
  */
 function _cleanUpEvents(elem, type) {
-  const data = DomData.getData(elem);
+  if (!DomData.has(elem)) {
+    return;
+  }
+  const data = DomData.get(elem);
 
   // Remove the events of a particular type if there are none left
   if (data.handlers[type].length === 0) {
@@ -48,7 +51,7 @@ function _cleanUpEvents(elem, type) {
 
   // Finally remove the element data if there is no data left
   if (Object.getOwnPropertyNames(data).length === 0) {
-    DomData.removeData(elem);
+    DomData.delete(elem);
   }
 }
 
@@ -250,7 +253,11 @@ export function on(elem, type, fn) {
     return _handleMultipleEvents(on, elem, type, fn);
   }
 
-  const data = DomData.getData(elem);
+  if (!DomData.has(elem)) {
+    DomData.set(elem, {});
+  }
+
+  const data = DomData.get(elem);
 
   // We need a place to store all our handler data
   if (!data.handlers) {
@@ -329,11 +336,11 @@ export function on(elem, type, fn) {
  */
 export function off(elem, type, fn) {
   // Don't want to add a cache object through getElData if not needed
-  if (!DomData.hasData(elem)) {
+  if (!DomData.has(elem)) {
     return;
   }
 
-  const data = DomData.getData(elem);
+  const data = DomData.get(elem);
 
   // If no events exist, nothing to unbind
   if (!data.handlers) {
@@ -405,7 +412,7 @@ export function trigger(elem, event, hash) {
   // Fetches element data and a reference to the parent (for bubbling).
   // Don't want to add a data object to cache for every parent,
   // so checking hasElData first.
-  const elemData = (DomData.hasData(elem)) ? DomData.getData(elem) : {};
+  const elemData = DomData.has(elem) ? DomData.get(elem) : {};
   const parent = elem.parentNode || elem.ownerDocument;
   // type = event.type || event,
   // handler;
@@ -432,7 +439,10 @@ export function trigger(elem, event, hash) {
 
   // If at the top of the DOM, triggers the default action unless disabled.
   } else if (!parent && !event.defaultPrevented && event.target && event.target[event.type]) {
-    const targetData = DomData.getData(event.target);
+    if (!DomData.has(event.target)) {
+      DomData.set(event.target, {});
+    }
+    const targetData = DomData.get(event.target);
 
     // Checks if the target has a default action for this event.
     if (event.target[event.type]) {

diff --git a/test/unit/component.test.js b/test/unit/component.test.js
@@ -2,7 +2,7 @@
 import window from 'global/window';
 import Component from '../../src/js/component.js';
 import * as Dom from '../../src/js/utils/dom.js';
-import * as DomData from '../../src/js/utils/dom-data';
+import DomData from '../../src/js/utils/dom-data';
 import * as Events from '../../src/js/utils/events.js';
 import * as Obj from '../../src/js/utils/obj';
 import * as browser from '../../src/js/utils/browser.js';
@@ -346,7 +346,7 @@ QUnit.test('should dispose of component and children', function(assert) {
     return true;
   });
   const el = comp.el();
-  const data = DomData.getData(el);
+  const data = DomData.get(el);
 
   let hasDisposed = false;
   let bubbles = null;
@@ -365,7 +365,7 @@ QUnit.test('should dispose of component and children', function(assert) {
   assert.ok(!comp.el(), 'component element was deleted');
   assert.ok(!child.children(), 'child children were deleted');
   assert.ok(!child.el(), 'child element was deleted');
-  assert.ok(!DomData.hasData(el), 'listener data nulled');
+  assert.ok(!DomData.has(el), 'listener data nulled');
   assert.ok(
     !Object.getOwnPropertyNames(data).length,
     'original listener data object was emptied'
@@ -979,7 +979,7 @@ QUnit.test('*AnimationFrame methods fall back to timers if rAF not supported', f
 QUnit.test('setTimeout should remove dispose handler on trigger', function(assert) {
   const comp = new Component(getFakePlayer());
   const el = comp.el();
-  const data = DomData.getData(el);
+  const data = DomData.get(el);
 
   comp.setTimeout(() => {}, 1);
 
@@ -996,7 +996,7 @@ QUnit.test('setTimeout should remove dispose handler on trigger', function(asser
 QUnit.test('requestAnimationFrame should remove dispose handler on trigger', function(assert) {
   const comp = new Component(getFakePlayer());
   const el = comp.el();
-  const data = DomData.getData(el);
+  const data = DomData.get(el);
   const oldRAF = window.requestAnimationFrame;
   const oldCAF = window.cancelAnimationFrame;
 

diff --git a/test/unit/menu.test.js b/test/unit/menu.test.js
@@ -1,5 +1,5 @@
 /* eslint-env qunit */
-import * as DomData from '../../src/js/utils/dom-data';
+import DomData from '../../src/js/utils/dom-data';
 import MenuButton from '../../src/js/menu/menu-button.js';
 import Menu from '../../src/js/menu/menu.js';
 import CaptionSettingsMenuItem from '../../src/js/control-bar/text-track-controls/caption-settings-menu-item';
@@ -137,7 +137,7 @@ QUnit.test('should remove old event listeners when the menu item adds to the new
    * A reusable collection of assertions.
    */
   function validateMenuEventListeners(watchedMenu) {
-    const eventData = DomData.getData(menuItem.eventBusEl_);
+    const eventData = DomData.get(menuItem.eventBusEl_);
     // `MenuButton`.`unpressButton` will be called when triggering click event on the menu item.
     const unpressButtonSpy = sinon.spy(menuButton, 'unpressButton');
     // `MenuButton`.`focus` will be called when triggering click event on the menu item.

diff --git a/test/unit/utils/dom-data.test.js b/test/unit/utils/dom-data.test.js
diff --git a/test/unit/videojs-integration.test.js b/test/unit/videojs-integration.test.js
@@ -2,7 +2,6 @@
 import videojs from '../../src/js/video.js';
 import window from 'global/window';
 import document from 'global/document';
-import * as DomData from '../../src/js/utils/dom-data';
 import * as Fn from '../../src/js/utils/fn';
 
 /**
@@ -70,54 +69,11 @@ QUnit.test('create a real player and dispose', function(assert) {
 
   player.muted(true);
 
-  const checkDomData = function() {
-    Object.keys(DomData.elData).forEach(function(elId) {
-      const data = DomData.elData[elId] || {};
-
-      Object.keys(data.handlers || {}).forEach(function(eventName) {
-        const listeners = data.handlers[eventName];
-        const uniqueList = [];
-
-        (listeners || []).forEach(function(listener) {
-          let add = true;
-
-          for (let i = 0; i < uniqueList.length; i++) {
-            const obj = uniqueList[i];
-
-            if (listener.og_ && listener.cx_ && obj.fn === listener.og_ && obj.cx === listener.cx_) {
-              add = false;
-              break;
-            }
-
-            if (listener.og_ && !listener.cx_ && obj.fn === listener.og_) {
-              add = false;
-              break;
-            }
-
-            if (!listener.og_ && !listener.cx_ && obj.fn === listener) {
-              add = false;
-              break;
-            }
-          }
-          const obj = {fn: listener.og_ || listener, cx: listener.cx_};
-
-          if (add) {
-            uniqueList.push(obj);
-            assert.ok(true, `${elId}/${eventName}/${obj.fn.name} is unique`);
-          } else {
-            assert.ok(false, `${elId}/${eventName}/${obj.fn.name} is not unique`);
-          }
-        });
-      });
-    });
-  };
-
   player.addTextTrack('captions', 'foo', 'en');
   player.ready(function() {
     assert.ok(player.tech_, 'tech exists');
     assert.equal(player.textTracks().length, 1, 'should have one text track');
 
-    checkDomData();
     player.dispose();
 
     Object.keys(old).forEach(function(k) {