Optimize vsync #2451
Optimize vsync #2451
Conversation
|
CC @dvoytenko |
| */ | ||
| run(task, opt_state) { | ||
| this.tasks_.push(task); | ||
| this.states_.push(opt_state || {}); | ||
| this.states_.push(opt_state); |
There was a problem hiding this comment.
That's a sizable change to the API. I'd expect something would break. I definitely see some places where default opt_state is used.
There was a problem hiding this comment.
The documentation states otherwise. No tests fail. Do you have a pointer?
There was a problem hiding this comment.
I found 2 places. Fixed them up.
This API sucks. Should have just let measure return an object.
- use double buffer for task and state arrays. - only allocate only promise per frame for measure and run promise. - don't allocate state object if not provided (as documentation already said). - use same task object shape throughout. - allocate one less closure in `runPromise`
cramforce
force-pushed
the
observable-mem
branch
from
fdf88a2 to
fd465c4
Compare
| @@ -309,6 +334,14 @@ export class Vsync { | |||
| tasks[i].mutate(states[i]); | |||
| } | |||
| } | |||
| // Swap last arrays into double buffer. | |||
| this.nextTasks_ = tasks; | |||
There was a problem hiding this comment.
I don't understand the point of having nextTasks_ if we just reassign to tasks and truncate.
There was a problem hiding this comment.
this.tasks_ can be modified during the loop above, so unless you want want to use a weird splice magic, you need 2 arrays.
There was a problem hiding this comment.
Ha, I totally read this backwards (as in you're truncating the old nextTasks_, not the new). Never mind.
|
LGTM. |
|
LGTM |
runPromise