Skip to content

[Fiber] Animation priority work#7466

Merged
sebmarkbage merged 3 commits intofacebook:masterfrom
acdlite:fiberhighpriority
Sep 13, 2016
Merged

[Fiber] Animation priority work#7466
sebmarkbage merged 3 commits intofacebook:masterfrom
acdlite:fiberhighpriority

Conversation

@acdlite
Copy link
Copy Markdown
Collaborator

@acdlite acdlite commented Aug 10, 2016

Adds the ability to schedule and perform high priority work. In the noop renderer, this is exposed using a method performHighPriWork(fn) where the function is executed and all updates in that scope are given high priority.

To do this, the scheduler keeps track of a default priority level. A new function performWithPriority(priority, fn) changes the default priority before calling the function, then resets it afterwards.

@sebmarkbage
Copy link
Copy Markdown
Contributor

Rebased: https://github.com/sebmarkbage/react/tree/fiberhighpriority (You can allow edits from maintainers now which is good for this use case. :) )

Comment thread src/renderers/noop/ReactNoop.js Outdated
},

performHighPriWork(fn: Function) {
NoopRenderer.performWithPriority(HighPriority, fn);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want to expose this full granularity in the API we expose to renderers. I've been thinking about a different API for scheduling that would be based on expiration date buckets. If we don't expose it to users, maybe we also shouldn't expose it to renderers?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I agree it's too much to expose to renderers. I only added it here because I needed some way to test it. Any suggestions?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If we don't expose it to users, maybe we also shouldn't expose it to renderers?

I can imagine renderers might need at least some more granular control than a user, because there are certain scheduling tricks that can only be controlled at the renderer level. E.g. I had assumed that the deprioritization that happens when hidden={true} (or style={{ display: hidden }}) would eventually be handled by the renderer via some API, rather than being embedded in Fiber itself. Or a renderer might want to use a different priority for specific types of events? Curious what your plans are for that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yea, I think the deprioritization case needs to be handled by the renderer somehow. However, I don't see a good way to expose it as an API to schedule a specific priority.

I think possibly that prepareUpdate could return the priority level. However, currently that happens after the children has already rendered. Would need to split children update from props update - which I want to do anyway.

It could also be an explicit API like isOffscreen(...) or something.

I'm not sure if the deprioritization use case make much sense for a renderer to do in another case than Offscreen so it probably isn't granular anyway.

@acdlite
Copy link
Copy Markdown
Collaborator Author

acdlite commented Sep 3, 2016

@sebmarkbage Rebased. I also checked the box to allow edits from maintainers on this and the other PRs :)

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.03%) to 87.217% when pulling 577409d on acdlite:fiberhighpriority into a09d158 on facebook:master.

@ghost ghost added the CLA Signed label Sep 3, 2016
if (defaultPriority === NoWork) {
return;
}
if (defaultPriority >= LowPriority) {
Copy link
Copy Markdown
Contributor

@sebmarkbage sebmarkbage Sep 6, 2016

Choose a reason for hiding this comment

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

This should actually be defaultPriority >= HighPriority.

We ended up overloading the term which needs to be undone. We should rename scheduleHighPriWork to scheduleAnimationWork because HighPriority is the highest priority that goes in to requestIdleCallback. We should only do AnimationPriority work during requestAnimationFrame.

Additionally, SynchronousPriority should not use scheduleHighPriWork so it should throw in that case. At least until we fix it.

@acdlite acdlite changed the title [Fiber] High priority work [Fiber] Animation priority work Sep 9, 2016
@ghost ghost added the CLA Signed label Sep 9, 2016
@ghost ghost added the CLA Signed label Sep 9, 2016
@acdlite acdlite force-pushed the fiberhighpriority branch 3 times, most recently from 6d1fff6 to d238f8c Compare September 9, 2016 20:17
@ghost ghost added the CLA Signed label Sep 9, 2016
@ghost ghost added the CLA Signed label Sep 10, 2016
acdlite and others added 3 commits September 13, 2016 15:37
Adds the ability to schedule and perform high priority work. In the
noop renderer, this is exposed using a method `performHighPriWork(fn)`
where the function is executed and all updates in that scope are given
high priority.

To do this, the scheduler keeps track of a default priority level.
A new function `performWithPriority(priority, fn)` changes the default
priority before calling the function, then resets it afterwards.
"High" and "low" priority are overloaded terms. There are priority
levels called HighPriority and LowPriority. Meanwhile, there are
functions called {perform,schedule}HighPriWork, which corresponds
to requestAnimationFrame, and {perform,schedule}LowPriWork, which
corresponds to requestIdleCallback. But in fact, work that has
HighPriority is meant to be scheduled with requestIdleCallback.
This is super confusing.

To address this, {perform,schedule}HighPriWork has been renamed
to {perform,schedule}AnimationWork, and
{perform,schedule}LowPriWork has been renamed to
{perform,schedule}DeferredWork. HighPriority and LowPriority
remain the same.
@sebmarkbage sebmarkbage merged commit 6144212 into facebook:master Sep 13, 2016
@zpao zpao modified the milestone: 15-next Sep 19, 2016
@zpao zpao removed this from the 15-next milestone Oct 4, 2016
@zpao zpao modified the milestones: 15.4.0, 15-next Oct 4, 2016
zpao pushed a commit that referenced this pull request Oct 4, 2016
* High priority work

Adds the ability to schedule and perform high priority work. In the
noop renderer, this is exposed using a method `performHighPriWork(fn)`
where the function is executed and all updates in that scope are given
high priority.

To do this, the scheduler keeps track of a default priority level.
A new function `performWithPriority(priority, fn)` changes the default
priority before calling the function, then resets it afterwards.

* Rename overloaded priority terms

"High" and "low" priority are overloaded terms. There are priority
levels called HighPriority and LowPriority. Meanwhile, there are
functions called {perform,schedule}HighPriWork, which corresponds
to requestAnimationFrame, and {perform,schedule}LowPriWork, which
corresponds to requestIdleCallback. But in fact, work that has
HighPriority is meant to be scheduled with requestIdleCallback.
This is super confusing.

To address this, {perform,schedule}HighPriWork has been renamed
to {perform,schedule}AnimationWork, and
{perform,schedule}LowPriWork has been renamed to
{perform,schedule}DeferredWork. HighPriority and LowPriority
remain the same.

* Priority levels merge fix

(cherry picked from commit 6144212)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants