perf: throttle more timers and use native bind #6142
Conversation
| @@ -88,7 +103,7 @@ class TimeTooltip extends Component { | |||
| /** | |||
| * Write the time to the tooltip DOM element. | |||
| * | |||
| * @param {String} content | |||
| * @param {string} content | |||
There was a problem hiding this comment.
I guess the linter fixed this one.
| style.width = percentage; | ||
| const sizeKey = this.vertical() ? 'height' : 'width'; | ||
|
|
||
| if (style[sizeKey] !== percentage) { |
There was a problem hiding this comment.
Only update on change, prevents a style recalculate
| const bound = function() { | ||
| return fn.apply(context, arguments); | ||
| }; | ||
| const bound = fn.bind(context); |
There was a problem hiding this comment.
Use the native bind function
There was a problem hiding this comment.
ooh, this likely will be super helpful in debugging.
There was a problem hiding this comment.
Yup, it removes an extra stack frame. Goes from data dispatcher directly to the method instead of through this bound method.
brandonocasey
force-pushed
the
perf/timers-and-raf
branch
from
12add95 to
1414906
Compare
|
Unfortunately, I don't think we can accept a large chunk of these changes as is. While a user is inactive, they could have still used their screen reader to focus on the seekbar or the time tooltip and would expect it to continue updating during playback. We probably want to do this for document visibility instead of user activity in the player. |
brandonocasey
force-pushed
the
perf/timers-and-raf
branch
from
1414906 to
0b91a71
Compare
brandonocasey
force-pushed
the
perf/timers-and-raf
branch
from
0b91a71 to
9cff4cc
Compare
brandonocasey
changed the title
| */ | ||
| constructor(player, options) { | ||
| super(player, options); | ||
| this.update = Fn.throttle(Fn.bind(this, this.update), 25); |
There was a problem hiding this comment.
in the seek bar, we throttle to 30ms: https://github.com/videojs/video.js/blob/master/src/js/control-bar/progress-control/seek-bar.js#L25, probably worth using the same thing everywhere. Maybe even have this value as a property exported from Fn utils?
There was a problem hiding this comment.
Yeah let's do that.
) Follow up from #6142 to include a couple of other uses.
Description
This pull request contains various performance improvement broken up into separate commits.