Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[unimodules-react-native-adapter] Flush UI blocks when needed #4125

Merged
merged 1 commit into from May 8, 2019

Conversation

sjchmiela
Copy link
Contributor

@sjchmiela sjchmiela commented May 3, 2019

Why

Fixes #2288.

How

Adding a UI block wasn't making React run the blocks as soon as possible. 馃し鈥嶁檪

Test Plan

With this fix applied Camera.takePicture works as expected.

@sjchmiela sjchmiela requested a review from ide May 3, 2019 19:41
@ide
Copy link
Member

ide commented May 4, 2019

setNeedsLayout actually performs layout and might be expensive on every addUIBlock call (I have not profiled this, though) -- alternatively, what do you think about mirroring UIManager's API and exposing a setNeedsLayout method that the Unimodule is responsible for calling?

@sjchmiela
Copy link
Contributor Author

From my short debugging investigation it looked like React doesn鈥檛 do much if the layout hasn鈥檛 changed. (In _layoutAndMount, the first _dispatches return early, uiBlockWithLayoutUpdate too.) Is there some other place layout is being recalculated? 馃

@ide
Copy link
Member

ide commented May 5, 2019

I didn't look into the implementation _layoutAndMount very deeply, so it may be cheap to call it. My main concerns with this PR are (a) whether _layoutAndMount is expensive to call frequently and (b) whether there are other non-performance reasons we want to add a UI block without triggering layout. (Why does RN's UIManager not run _layoutAndMount when adding a UI block?)

@sjchmiela
Copy link
Contributor Author

a) _layoutAndMount seems to be cheap to call frequently
b) Other React Native libraries dispatch calls to view managers via UIManager.dispatchViewManagerCommand, which somehow ensures that batch of calls is executed right away and afterwards, batchDidComplete is called, which triggers _layoutAndMount on RCTUIManager.

I think we could either change addUIBlock into executeUIBlock, which would 1. add and then 2. trigger _layoutAndMount (similar to UIManager.dispatchViewManagerCommand, but probably not batched) or use UIM.dVMC in our unimodules' code (like other libraries do), but then write our own implementation of handling Promises for these functions.

Copy link
Member

@ide ide left a comment

Choose a reason for hiding this comment

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

change addUIBlock into executeUIBlock

I think this is a good idea -- we probably want a native API to add a block in addition to a JS API.

@sjchmiela sjchmiela force-pushed the @sjchmiela/flush_ui_blocks branch from 3fb5ada to fb9af16 Compare May 7, 2019 15:01
@sjchmiela sjchmiela changed the title [unimodules-react-native-adapter] Always flush UI blocks after adding one [unimodules-react-native-adapter] Run UI blocks synchronously May 7, 2019
@sjchmiela
Copy link
Contributor Author

What do you think about running UI blocks synchronously on the main thread?

@ide
Copy link
Member

ide commented May 7, 2019

I think it'd be better to execute the UI blocks asynchronously by default for a few reasons:

  • dispatch_sync can lead to deadlock. If we need synchronous execution, I think we should have a separate way to do that, ex: introduce a new method like [UIManager dangerouslyExecuteUIBlockSynchronously].
  • If there are other UI blocks in the queue, we would run all of them. This might be a little expensive, and it'd be better to run the UI blocks at the start of the next frame so we have a full 16ms to run them.
  • Generally, it is easier to change asynchronous code to be synchronous code instead of the other direction. So if we decide that synchronous execution makes more sense, perhaps with Fabric, it will be easier to make that change.

Additionally, it looks like UIManager handles running UI blocks in flushUIBlocksWithCompletion. It gathers all the currently pending UI blocks, and then executes them on the main thread (with dispatch_async). Since that behavior works for the rest of RN, I'd naively expect it to work for Unimodules too.

@sjchmiela sjchmiela force-pushed the @sjchmiela/flush_ui_blocks branch from fb9af16 to 9bf2cec Compare May 7, 2019 19:55
@sjchmiela sjchmiela force-pushed the @sjchmiela/flush_ui_blocks branch from 9bf2cec to 5dd9925 Compare May 7, 2019 19:57
@sjchmiela sjchmiela changed the title [unimodules-react-native-adapter] Run UI blocks synchronously [unimodules-react-native-adapter] Flush UI blocks when needed May 7, 2019
@sjchmiela sjchmiela merged commit 87558b0 into master May 8, 2019
@sjchmiela sjchmiela deleted the @sjchmiela/flush_ui_blocks branch May 8, 2019 05:10
Jamedjo pushed a commit to Jamedjo/expo that referenced this pull request Feb 4, 2021
)

# Why

Fixes expo#2288.

# How

Adding a UI block wasn't making React run the blocks as soon as possible. 馃し鈥嶁檪 

# Test Plan

With this fix applied `Camera.takePicture` works as expected.
prakashbask pushed a commit to prakashbask/expo that referenced this pull request Mar 16, 2022
)

# Why

Fixes expo#2288.

# How

Adding a UI block wasn't making React run the blocks as soon as possible. 馃し鈥嶁檪 

# Test Plan

With this fix applied `Camera.takePicture` works as expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Camera] camera.takePictureAsync() responds only after second call
2 participants