Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

use setImmediate instead of requestAnimationFrame on Node.js #1145

Merged
merged 2 commits into from
Jul 10, 2018

Conversation

ChALkeR
Copy link
Contributor

@ChALkeR ChALkeR commented Jul 9, 2018

Description

This makes await tf.nextFrame(); work on Node.js for better code interoperability.

It also does the expected thing of unblocking the event loop and giving the engine time to perform other tasks, though the name is a bit confusing.

The order of the check is important: IE and Edge also implement setImmediate, but all versions that do that also have requestAnimationFrame, so requestAnimationFrame is used on those.

Refs: setImmediate, Node.js Event Loop.


For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md


This change is Reviewable

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Jul 9, 2018

As a side note, it might be a good thing to use requestIdleCallback instead of requestAnimationFrame on browsers that implement it (currently FF and Chrome), but that is not included here.

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @ChALkeR)


src/browser_util.ts, line 22 at r1 (raw file):

  ? requestAnimationFrame // Browsers
  : setImmediate; // Node.js

This is great! Can you add a simple unit test that calls tf.nextFrame() and asserts that it calls a method? Introduce a browser_util_test.ts and use describeWithFlags('browser_util', ALL_ENVS, () => {}) which is a custom describe that runs on all envs.

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Jul 10, 2018

@dsmilkov I added tests, but using describeWithFlags('nextFrame' — that looks to be more in line with the other tests.

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Even better. Thanks!

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@dsmilkov dsmilkov merged commit 37486bc into tensorflow:master Jul 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants