Delay Penalized Elements After First Visible Time #2671
Conversation
[Perf](http://jsperf.com/date-now-vs-new-date/20) is better, and it’s [supported in everything >= IE 9](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Glo bal_Objects/Date/now#Browser_compatibility)
| @@ -47,8 +47,7 @@ export class Timer { | |||
| * @return {number} | |||
| */ | |||
| now() { | |||
| // TODO(dvoytenko): when can we use Date.now? | |||
| return Number(new Date()); | |||
| return Date.now(); | |||
|
@jridgewell conceptually looks great and, as mentioned offline, I believe this solves more than one problem. |
jridgewell
force-pushed
the
resources-cleanup
branch
from
2471822
to
4be2586
Compare
|
PTAL. Should we be adding an integration test for this? |
| if (this.exec_.getSize() == 0) { | ||
| return 0; | ||
| if (this.firstVisibleTime_ === -1) { |
|
@jridgewell Would like more integration tests for resources.js in general, but not really this case - it plays very well on the execution timeout and was always meant to be very unit-testable. |
| @@ -126,6 +126,11 @@ describe('Resources', () => { | |||
| expect(resources.calcTaskTimeout_(task_p0)).to.equal(0); | |||
| expect(resources.calcTaskTimeout_(task_p1)).to.equal(0); | |||
|
|
|||
| // Idle render penalty after first visible | |||
| resources.firstVisibleTime_ = 0; | |||
There was a problem hiding this comment.
Let's add a test for firstVisibleTime_ == -1
There was a problem hiding this comment.
That's implicit in the "Empty pool" tests above. Would you like me to add the explicit resources.firstVisibleTime_ = -1 there?
|
@jridgewell This is all good. I'm pretty much at LGTM. Just couple of comments - please address and I'll do another pass tomorrow morning. |
jridgewell
force-pushed
the
resources-cleanup
branch
from
4be2586
to
6db476d
Compare
| return 0; | ||
| // If we've never been visible, return 0. This follows the previous | ||
| // behavior of not delaying tasks when there's nothing to do. | ||
| // TODO(jridgewell): Should we return an un-subtracted `penalty` here? |
There was a problem hiding this comment.
I don't think TODO is needed here. The processing is done via two queues: scheduling and execution. We delay execution here which is fine. If we have an issue with INVISIBLE mode - that should be addressed in scheduling and it already is - i.e. we do not schedule elements unless they opt-in and close to viewport, etc.
|
LGTM with one last comment. |
In the first moments after first visible time, we will delay penalized elements (based on their element priority). Once the document has been visible for more than the element’s priority delay, we allow them to run.
jridgewell
force-pushed
the
resources-cleanup
branch
from
6db476d
to
bbea7b3
Compare
In the first moments after first visible time, we will delay penalized elements (based on their element priority).
If the document has already been visible for a while, no penalty will be added.
Fixes #2561.