Initial implementation of requestIdleCallback on Android #5052

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants
@brentvatne
Collaborator

brentvatne commented Dec 30, 2015

  • Does not implement implement IdleRequestOptions or didTimeout
  • Uses naive heuristic for finding idle time: runs after all ReactChoreographer callbacks, if there is >= 12ms remaining in frame, execute idle callbacks
  • Falls back to requestAnimationFrame on iOS and warns that it is not implemented on this platform yet

Test plan:

  • Use requestIdleCallback to schedule some work, then stagger CPU to be burned for ~10ms in various requestAnimationFrame callbacks, idle callback should not fire until all of those are completed.

ric

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 30, 2015

By analyzing the blame information on this pull request, we identified @mkonicek, @spicyj and @mhorowitz to be potential reviewers.

By analyzing the blame information on this pull request, we identified @mkonicek, @spicyj and @mhorowitz to be potential reviewers.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 31, 2015

@brentvatne updated the pull request.

@brentvatne updated the pull request.

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Dec 31, 2015

Contributor

Hey @brentvatne note that your unit tests are crashing, check out ./gradlew :ReactAndroid:testDebugUnitTest

Contributor

bestander commented Dec 31, 2015

Hey @brentvatne note that your unit tests are crashing, check out ./gradlew :ReactAndroid:testDebugUnitTest

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Dec 31, 2015

Contributor

I am working on making CI builds stable and your branch caught my attention.

Contributor

bestander commented Dec 31, 2015

I am working on making CI builds stable and your branch caught my attention.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jan 1, 2016

Collaborator

Thanks @bestander - I'll check that out :)

Collaborator

brentvatne commented Jan 1, 2016

Thanks @bestander - I'll check that out :)

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jan 3, 2016

Collaborator

@bestander - summary of the issue I'm having getting TimingModuleTest to pass follows:

  • Currently the only test that isn't working is testPausingAndResuming
  • On my machine, test execution simply freezes and does not throw any sort of error to indicate why it isn't working
  • I looked into this further and found that mTiming.onHostPause(); is the cause
  • If I comment this line out, it passes
Collaborator

brentvatne commented Jan 3, 2016

@bestander - summary of the issue I'm having getting TimingModuleTest to pass follows:

  • Currently the only test that isn't working is testPausingAndResuming
  • On my machine, test execution simply freezes and does not throw any sort of error to indicate why it isn't working
  • I looked into this further and found that mTiming.onHostPause(); is the cause
  • If I comment this line out, it passes
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 3, 2016

@brentvatne updated the pull request.

@brentvatne updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 3, 2016

@brentvatne updated the pull request.

@brentvatne updated the pull request.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jan 3, 2016

Collaborator

Nevermind @bestander - I figured it out in 2c4c39a and added coverage for requestIdleCallback in 8d8da11 :)

Collaborator

brentvatne commented Jan 3, 2016

Nevermind @bestander - I figured it out in 2c4c39a and added coverage for requestIdleCallback in 8d8da11 :)

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Jan 3, 2016

Contributor

👍 well done.
Don't worry about one failing test, someone has broken it during the weekend, I'll chase them.

Contributor

bestander commented Jan 3, 2016

👍 well done.
Don't worry about one failing test, someone has broken it during the weekend, I'll chase them.

@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Jan 5, 2016

Contributor

Unfortunately, this doesn't give us much of a guarantee of idleness: Choreographer (and ReactChoreographer) callbacks execute their callbacks in the CALLBACK_ANIMATION phase of the frame, before we do CALLBACK_TRAVERSALS (the standard layout/measure/draw pass): https://github.com/android/platform_frameworks_base/blob/d59921149bb5948ffbcb9a9e832e9ac1538e05a0/core/java/android/view/Choreographer.java#L600-L608. Additionally (though probably less significant), any messages that were posted to the Looper (e.g. via View#post) since the last frame's CALLBACK_TRAVERSALS was called will be executed after the current frame's CALLBACK_TRAVERSALS.

Can you give an example of what you're trying to accomplish and maybe I can give some suggestions?

Contributor

astreet commented Jan 5, 2016

Unfortunately, this doesn't give us much of a guarantee of idleness: Choreographer (and ReactChoreographer) callbacks execute their callbacks in the CALLBACK_ANIMATION phase of the frame, before we do CALLBACK_TRAVERSALS (the standard layout/measure/draw pass): https://github.com/android/platform_frameworks_base/blob/d59921149bb5948ffbcb9a9e832e9ac1538e05a0/core/java/android/view/Choreographer.java#L600-L608. Additionally (though probably less significant), any messages that were posted to the Looper (e.g. via View#post) since the last frame's CALLBACK_TRAVERSALS was called will be executed after the current frame's CALLBACK_TRAVERSALS.

Can you give an example of what you're trying to accomplish and maybe I can give some suggestions?

@@ -155,7 +226,9 @@ private void setChoreographerCallback() {
if (!mFrameCallbackPosted) {
Assertions.assertNotNull(mReactChoreographer).postFrameCallback(
ReactChoreographer.CallbackType.TIMERS_EVENTS,
- mFrameCallback);
+ mTimerFrameCallback);
+ Assertions.assertNotNull(mChoreographer).postFrameCallback(

This comment has been minimized.

@astreet

astreet Jan 5, 2016

Contributor

We really don't want to use the regular Choreographer here: ReactChoreographer allows us to be explicit about ordering of callback execution within the React framework, whereas if we use Choreographer directly, we might accidentally execute this before all of the ReactChoreographer callbacks.

@astreet

astreet Jan 5, 2016

Contributor

We really don't want to use the regular Choreographer here: ReactChoreographer allows us to be explicit about ordering of callback execution within the React framework, whereas if we use Choreographer directly, we might accidentally execute this before all of the ReactChoreographer callbacks.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jan 5, 2016

Collaborator

Unfortunately, this doesn't give us much of a guarantee of idleness: Choreographer (and ReactChoreographer) callbacks execute their callbacks in the CALLBACK_ANIMATION phase of the frame, before we do CALLBACK_TRAVERSALS (the standard layout/measure/draw pass): https://github.com/android/platform_frameworks_base/blob/d59921149bb5948ffbcb9a9e832e9ac1538e05a0/core/java/android/view/Choreographer.java#L600-L608. Additionally (though probably less significant), any messages that were posted to the Looper (e.g. via View#post) since the last frame's CALLBACK_TRAVERSALS was called will be executed after the current frame's CALLBACK_TRAVERSALS.

(More info)

Some notes to make up for my really poorly documented description at the top of this PR:

  • This is a rough implementation that is intended to be experimental - the API shouldn't change (except for adding a timeout option) but the underlying behaviour can.
  • Thanks for the details! This is my first time using the Choreographer API -- it looks like it makes more sense for us to put this after commit then, and that would be in agreement with diagram above. I couldn't find in the Choreographer source any way for us to add frame callback to CALLBACK_COMMIT -- is this what we would want to do or is there another way to do this?

We really don't want to use the regular Choreographer here: ReactChoreographer allows us to be explicit about ordering of callback execution within the React framework, whereas if we use Choreographer directly, we might accidentally execute this before all of the ReactChoreographer callbacks.

Makes sense! I had it in my head that because I needed it to be outside of ReactChoreographer for reasons I can't quite remember because I originally wrote this about 2 months ago.

Collaborator

brentvatne commented Jan 5, 2016

Unfortunately, this doesn't give us much of a guarantee of idleness: Choreographer (and ReactChoreographer) callbacks execute their callbacks in the CALLBACK_ANIMATION phase of the frame, before we do CALLBACK_TRAVERSALS (the standard layout/measure/draw pass): https://github.com/android/platform_frameworks_base/blob/d59921149bb5948ffbcb9a9e832e9ac1538e05a0/core/java/android/view/Choreographer.java#L600-L608. Additionally (though probably less significant), any messages that were posted to the Looper (e.g. via View#post) since the last frame's CALLBACK_TRAVERSALS was called will be executed after the current frame's CALLBACK_TRAVERSALS.

(More info)

Some notes to make up for my really poorly documented description at the top of this PR:

  • This is a rough implementation that is intended to be experimental - the API shouldn't change (except for adding a timeout option) but the underlying behaviour can.
  • Thanks for the details! This is my first time using the Choreographer API -- it looks like it makes more sense for us to put this after commit then, and that would be in agreement with diagram above. I couldn't find in the Choreographer source any way for us to add frame callback to CALLBACK_COMMIT -- is this what we would want to do or is there another way to do this?

We really don't want to use the regular Choreographer here: ReactChoreographer allows us to be explicit about ordering of callback execution within the React framework, whereas if we use Choreographer directly, we might accidentally execute this before all of the ReactChoreographer callbacks.

Makes sense! I had it in my head that because I needed it to be outside of ReactChoreographer for reasons I can't quite remember because I originally wrote this about 2 months ago.

@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Jan 6, 2016

Contributor

So, the important thing I wasn't really thinking about was that we are just concerned about idle time on the JS thread, not the UI thread. At least for now. Trying to estimate time remaining on the JS thread + time remaining on the UI thread for the (next) frame in which those resulting native UIManager module calls would be processed on the main thread is basically impossible.

So we can be satisfied for now by just invoking idle callbacks once we think we're done sending JS calls for the frame. These should be executed after all the CallbackType.TIMERS_EVENTS callbacks have executed (i.e. the Events that make JS do things.), so we should just add a new CallbackType in ReactChoreographer.

Note, however, that these Choreographer callbacks execute on the main thread. We may have already enqueued all the events and timers we need to call on the JS event queue, but we don't yet know at this time how long they'll take to execute. It could be no time, it could be multiple frames. This means that the idle handler in JS will need to actually check the amount of time remaining in the frame when it actually executes: that time could be zero or negative.

Finally, in the case where there are JS events/timers that take multiple frames to execute, we don't want to back up the JS event queue with a useless idle callback on each frame. To deal with this, I suggest we do what we do in EventDispatcher where we make sure to only have a single pending runnable in the queue at any point in time:

if (!mHasDispatchScheduled) {
mHasDispatchScheduled = true;
Systrace.startAsyncFlow(
Systrace.TRACE_TAG_REACT_JAVA_BRIDGE,
"ScheduleDispatchFrameCallback",
mHasDispatchScheduledCount);
mReactContext.runOnJSQueueThread(mDispatchEventsRunnable);

Contributor

astreet commented Jan 6, 2016

So, the important thing I wasn't really thinking about was that we are just concerned about idle time on the JS thread, not the UI thread. At least for now. Trying to estimate time remaining on the JS thread + time remaining on the UI thread for the (next) frame in which those resulting native UIManager module calls would be processed on the main thread is basically impossible.

So we can be satisfied for now by just invoking idle callbacks once we think we're done sending JS calls for the frame. These should be executed after all the CallbackType.TIMERS_EVENTS callbacks have executed (i.e. the Events that make JS do things.), so we should just add a new CallbackType in ReactChoreographer.

Note, however, that these Choreographer callbacks execute on the main thread. We may have already enqueued all the events and timers we need to call on the JS event queue, but we don't yet know at this time how long they'll take to execute. It could be no time, it could be multiple frames. This means that the idle handler in JS will need to actually check the amount of time remaining in the frame when it actually executes: that time could be zero or negative.

Finally, in the case where there are JS events/timers that take multiple frames to execute, we don't want to back up the JS event queue with a useless idle callback on each frame. To deal with this, I suggest we do what we do in EventDispatcher where we make sure to only have a single pending runnable in the queue at any point in time:

if (!mHasDispatchScheduled) {
mHasDispatchScheduled = true;
Systrace.startAsyncFlow(
Systrace.TRACE_TAG_REACT_JAVA_BRIDGE,
"ScheduleDispatchFrameCallback",
mHasDispatchScheduledCount);
mReactContext.runOnJSQueueThread(mDispatchEventsRunnable);

@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Jan 6, 2016

Contributor

I think the pattern for using this API should be:

// Taken from https://developers.google.com/web/updates/2015/08/using-requestidlecallback?hl=en
function myNonEssentialWork (deadline) {
  while (deadline.timeRemaining() > 0 && tasks.length > 0)
    doWorkIfNeeded();

  if (tasks.length > 0)
    requestIdleCallback(myNonEssentialWork);
}

Though experimental, a major part of this API will be figuring out how to ensure that the work you want to do in idle periods can be broken down into small/pauseable chunks. What may seem like ok chunk sizes on the emulator or on a phone with no load may bust the frame deadline on other phones.

Contributor

astreet commented Jan 6, 2016

I think the pattern for using this API should be:

// Taken from https://developers.google.com/web/updates/2015/08/using-requestidlecallback?hl=en
function myNonEssentialWork (deadline) {
  while (deadline.timeRemaining() > 0 && tasks.length > 0)
    doWorkIfNeeded();

  if (tasks.length > 0)
    requestIdleCallback(myNonEssentialWork);
}

Though experimental, a major part of this API will be figuring out how to ensure that the work you want to do in idle periods can be broken down into small/pauseable chunks. What may seem like ok chunk sizes on the emulator or on a phone with no load may bust the frame deadline on other phones.

@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Jan 6, 2016

Contributor

I would also say that we should have only one idle callback actually registered natively if there are one or more idle callbacks registered in JS land, for simplicity. JS can handle turning that idle callback into multiple callbacks if necessary.

Contributor

astreet commented Jan 6, 2016

I would also say that we should have only one idle callback actually registered natively if there are one or more idle callbacks registered in JS land, for simplicity. JS can handle turning that idle callback into multiple callbacks if necessary.

@hufeng

This comment has been minimized.

Show comment
Hide comment

hufeng commented Jan 14, 2016

cool

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jan 14, 2016

Collaborator

I would also say that we should have only one idle callback actually registered natively if there are one or more idle callbacks registered in JS land, for simplicity. JS can handle turning that idle callback into multiple callbacks if necessary.

That's a great point!

Thanks for the insights @astreet, I'll try to improve this and push up changes over the next couple of weeks.

Collaborator

brentvatne commented Jan 14, 2016

I would also say that we should have only one idle callback actually registered natively if there are one or more idle callbacks registered in JS land, for simplicity. JS can handle turning that idle callback into multiple callbacks if necessary.

That's a great point!

Thanks for the insights @astreet, I'll try to improve this and push up changes over the next couple of weeks.

+ },
+
+ _run: function(shouldBurnCPU) {
+ requestIdleCallback((deadline) => {

This comment has been minimized.

@eslint-bot

eslint-bot Mar 11, 2016

identifier requestIdleCallback Could not resolve name

@eslint-bot

eslint-bot Mar 11, 2016

identifier requestIdleCallback Could not resolve name

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 15, 2016

@brentvatne updated the pull request.

@brentvatne updated the pull request.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 15, 2016

Contributor

@brentvatne Is this ready for review now?

Contributor

mkonicek commented Mar 15, 2016

@brentvatne Is this ready for review now?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 26, 2016

@mkonicek would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

ghost commented Mar 26, 2016

@mkonicek would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Mar 27, 2016

Collaborator

This isn't yet ready for review, still need to address feedback.

Collaborator

ide commented Mar 27, 2016

This isn't yet ready for review, still need to address feedback.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Apr 7, 2016

Contributor

👍 Thanks for the clarification @ide, I wasn't sure what the status was.

Contributor

mkonicek commented Apr 7, 2016

👍 Thanks for the clarification @ide, I wasn't sure what the status was.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 23, 2016

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @mhorowitz as a potential reviewer. Could you take a look please or cc someone with more context?

ghost commented May 23, 2016

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @mhorowitz as a potential reviewer. Could you take a look please or cc someone with more context?

[Timers] Initial implementation of requestIdleCallback on Android
- Add requestIdleCallback/cancelIdleCallback to eslintrc
- Add test coverage for requestIdleCallback
+ },
+
+ _run: function(shouldBurnCPU) {
+ requestIdleCallback((deadline) => {

This comment has been minimized.

@eslint-bot

eslint-bot Jul 1, 2016

identifier requestIdleCallback Could not resolve name

@eslint-bot

eslint-bot Jul 1, 2016

identifier requestIdleCallback Could not resolve name

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jul 6, 2016

Collaborator

Moved to #8569

Collaborator

brentvatne commented Jul 6, 2016

Moved to #8569

@brentvatne brentvatne closed this Jul 6, 2016

ghost pushed a commit that referenced this pull request Jul 14, 2016

Initial implementation of requestIdleCallback on Android
Summary:
This is a follow up of the work by brentvatne in #5052. This addresses the feedback by astreet.

- Uses ReactChoreographer with a new callback type
- Callback dispatch logic moved to JS
- Only calls into JS when needed, when there are pending callbacks, it even removes the Choreographer listener when no JS context listen for idle events.

** Test plan **
Tested by running a background task that burns all remaining idle time (see new UIExplorer example) and made sure that UI and JS fps stayed near 60 on a real device (Nexus 6) with dev mode disabled. Also tried adding a JS driven animation and it stayed smooth.

Tested that native only calls into JS when there are pending idle callbacks.

Also tested that timers are executed before idle callback.
```
requestIdleCallback(() => console.log(1));
setTimeout(() => console.log(2), 100);
burnCPU(1000);
// 2
// 1
```

I did *not* test with webworkers but it should work as I'm using executor tokens.
Closes #8569

Differential Revision: D3558869

Pulled By: astreet

fbshipit-source-id: 61fa82eb26001d2b8c2ea69c35bf3eb5ce5454ba

samerce added a commit to iodine/react-native that referenced this pull request Aug 23, 2016

Initial implementation of requestIdleCallback on Android
Summary:
This is a follow up of the work by brentvatne in #5052. This addresses the feedback by astreet.

- Uses ReactChoreographer with a new callback type
- Callback dispatch logic moved to JS
- Only calls into JS when needed, when there are pending callbacks, it even removes the Choreographer listener when no JS context listen for idle events.

** Test plan **
Tested by running a background task that burns all remaining idle time (see new UIExplorer example) and made sure that UI and JS fps stayed near 60 on a real device (Nexus 6) with dev mode disabled. Also tried adding a JS driven animation and it stayed smooth.

Tested that native only calls into JS when there are pending idle callbacks.

Also tested that timers are executed before idle callback.
```
requestIdleCallback(() => console.log(1));
setTimeout(() => console.log(2), 100);
burnCPU(1000);
// 2
// 1
```

I did *not* test with webworkers but it should work as I'm using executor tokens.
Closes facebook#8569

Differential Revision: D3558869

Pulled By: astreet

fbshipit-source-id: 61fa82eb26001d2b8c2ea69c35bf3eb5ce5454ba

mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016

Initial implementation of requestIdleCallback on Android
Summary:
This is a follow up of the work by brentvatne in #5052. This addresses the feedback by astreet.

- Uses ReactChoreographer with a new callback type
- Callback dispatch logic moved to JS
- Only calls into JS when needed, when there are pending callbacks, it even removes the Choreographer listener when no JS context listen for idle events.

** Test plan **
Tested by running a background task that burns all remaining idle time (see new UIExplorer example) and made sure that UI and JS fps stayed near 60 on a real device (Nexus 6) with dev mode disabled. Also tried adding a JS driven animation and it stayed smooth.

Tested that native only calls into JS when there are pending idle callbacks.

Also tested that timers are executed before idle callback.
```
requestIdleCallback(() => console.log(1));
setTimeout(() => console.log(2), 100);
burnCPU(1000);
// 2
// 1
```

I did *not* test with webworkers but it should work as I'm using executor tokens.
Closes facebook#8569

Differential Revision: D3558869

Pulled By: astreet

fbshipit-source-id: 61fa82eb26001d2b8c2ea69c35bf3eb5ce5454ba
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment