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.

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

perf: Another 5ms of startup time improvements #6145

Merged
merged 2 commits into from
Aug 29, 2019
Merged

Conversation

brandonocasey
Copy link
Contributor

See code comments

src/js/component.js Outdated Show resolved Hide resolved
@@ -57,7 +56,7 @@ class ErrorDisplay extends ModalDialog {
*
* @private
*/
ErrorDisplay.prototype.options_ = mergeOptions(ModalDialog.prototype.options_, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Object.assign is much faster.

@@ -55,7 +53,7 @@ const REMOTE = {
}
};

const ALL = mergeOptions(NORMAL, REMOTE);
const ALL = Object.assign({}, NORMAL, REMOTE);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Object.assign is much faster.

src/js/utils/dom.js Outdated Show resolved Hide resolved
@@ -160,7 +160,7 @@ export function createEl(tagName = 'div', properties = {}, attributes = {}, cont
// method for it.
} else if (propName === 'textContent') {
textContent(el, val);
} else {
} else if (el[propName] !== val) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is also another big startup time user. We should make sure that the prop is not already equal to what we are going to set it to before doing it.

@@ -84,6 +84,9 @@ function _handleMultipleEvents(fn, elem, types, callback) {
* Fixed event object.
*/
export function fixEvent(event) {
if (event.fixed_) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This costs us 0.1 ms per event, but some event have it done to them 5+ times. Might as well only do it once.

Copy link
Member

Choose a reason for hiding this comment

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

Do we always go into the line 101/104 block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like we almost always do yeah.

});
let _supportsPassive;

const supportsPassive = function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason this takes about 2ms to complete, making it lazy makes it take much less time later when an event that would need to be passive is actual used.

['featuresNativeTextTracks', 'supportsNativeTextTracks'],
['featuresNativeVideoTracks', 'supportsNativeVideoTracks'],
['featuresNativeAudioTracks', 'supportsNativeAudioTracks']
].forEach(function(array) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

altogether these take about 1ms at startup.

@brandonocasey brandonocasey changed the title perf: Another 4ms of startup time improvements perf: Another 5ms of startup time improvements Jul 30, 2019
@brandonocasey brandonocasey force-pushed the perf/startup-time branch 2 times, most recently from 59e1f65 to e730ecd Compare August 1, 2019 15:32
src/js/component.js Outdated Show resolved Hide resolved
src/js/utils/dom.js Outdated Show resolved Hide resolved
src/js/utils/dom.js Outdated Show resolved Hide resolved
src/js/utils/dom.js Show resolved Hide resolved
@@ -84,6 +84,9 @@ function _handleMultipleEvents(fn, elem, types, callback) {
* Fixed event object.
*/
export function fixEvent(event) {
if (event.fixed_) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we always go into the line 101/104 block?

@@ -566,5 +567,7 @@ videojs.dom = Dom;
*/
videojs.url = Url;

videojs.defineLazyProperty = defineLazyProperty;
Copy link
Member

Choose a reason for hiding this comment

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

any reason to expose it currently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we will need it in a few other places, vhs for sure.

src/js/tech/html5.js Outdated Show resolved Hide resolved
@gkatsev gkatsev added the minor This PR can be added to a minor release. It should not be added to a patch release. label Aug 7, 2019
@gkatsev gkatsev merged commit 22782b8 into master Aug 29, 2019
@gkatsev gkatsev deleted the perf/startup-time branch August 29, 2019 20:44
zmousm added a commit to zmousm/video.js that referenced this pull request Oct 3, 2020
Skipping the tabIndex property on created elements due to videojs#6145
optimizations blocks them from receiving keyboard events, due to not
being focusable; for example this breaks closing ModalDialog elements by
pressing Escape.
Fix this by always setting tabIndex, as the element may return the same
value even though the property has not been explicitly set.

Fixes videojs#6870
gkatsev pushed a commit that referenced this pull request Nov 10, 2020
Skipping the tabIndex property on created elements due to #6145
optimizations blocks them from receiving keyboard events, due to not
being focusable; for example this breaks closing ModalDialog elements by
pressing Escape.
Fix this by always setting tabIndex, as the element may return the same
value even though the property has not been explicitly set.

Fixes #6870
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
Skipping the tabIndex property on created elements due to videojs#6145
optimizations blocks them from receiving keyboard events, due to not
being focusable; for example this breaks closing ModalDialog elements by
pressing Escape.
Fix this by always setting tabIndex, as the element may return the same
value even though the property has not been explicitly set.

Fixes videojs#6870
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed minor This PR can be added to a minor release. It should not be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants