Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

perf: Use weakmap for dom data #6103

Merged
merged 6 commits into from
Aug 1, 2019
Merged

perf: Use weakmap for dom data #6103

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
96f64c9
perf: Use weakmap for dom data
brandonocasey Jul 18, 2019
13d4675
use it like a weakmap
brandonocasey Jul 18, 2019
0d1b594
remove from vjs
brandonocasey Jul 18, 2019
1bd13fd
only a weakmap
brandonocasey Jul 18, 2019
32cd08f
bring the comment back
brandonocasey Jul 18, 2019
2f623f6
cr
brandonocasey Jul 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/js/component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ 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 * as Fn from './utils/fn.js';
import * as Guid from './utils/guid.js';
import toTitleCase from './utils/to-title-case.js';
Expand Down Expand Up @@ -152,7 +151,6 @@ class Component {
this.el_.parentNode.removeChild(this.el_);
}

DomData.removeData(this.el_);
brandonocasey marked this conversation as resolved.
Show resolved Hide resolved
this.el_ = null;
}

Expand Down
85 changes: 1 addition & 84 deletions src/js/utils/dom-data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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();
26 changes: 18 additions & 8 deletions src/js/utils/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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) {
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a couple usages of if !has, set, then get. Do you think it's worth a helper?

Copy link
Contributor Author

@brandonocasey brandonocasey Jul 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it might not make sense in this case since it is only used in two places.

EDIT: And the helper name will be likely have to be longer than duplicating the code would be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right. If we end up with this a few more times, then it'll make sense.

DomData.set(elem, {});
}

const data = DomData.get(elem);

// We need a place to store all our handler data
if (!data.handlers) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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]) {
Expand Down
10 changes: 5 additions & 5 deletions test/unit/component.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand All @@ -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'
Expand Down Expand Up @@ -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);

Expand All @@ -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;

Expand Down
4 changes: 2 additions & 2 deletions test/unit/menu.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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.
Expand Down
63 changes: 0 additions & 63 deletions test/unit/utils/dom-data.test.js
View file
Open in desktop

This file was deleted.

44 changes: 0 additions & 44 deletions test/unit/videojs-integration.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand Down Expand Up @@ -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) {
Expand Down