Skip to content

Commit

Permalink
Document early ViewCommand dispatch, since it is no longer gated
Browse files Browse the repository at this point in the history
Summary:
In 0b63c94, I shipped "Early ViewCommand Dispatch" everywhere but didn't provide a good summary in the commit message.

Here it is:

The "Early ViewCommand Dispatch" experiment was a success, leading to improved UI responsiveness and fewer overall crashes.

Ship to all Fabric and non-Fabric users, and clean up the code a bit.

# Context: what?

ViewCommands are now used in React Native to do things like scroll ScrollViews to a certain position; focus or blur a TextInput; control the value of controlled TextInputs; and much more. These used to be done using setNativeProps, but we're moving everything to ViewCommands, and most of that migration has already finished.

# Context: Why?

Because Fabric does not support setNativeProps, there has been an effort to move all setNativeProps callsites to ViewCommands. Since these callsites are in JS where we can't tell if we're running in Paper or Fabric, both Paper and Fabric callsites are being migrated (mostly already done) to ViewCommands. One such example is Android TextInput, which has been using ViewCommands instead of setNativeProps for several months.

This migration was largely without issues, except TheSavior and I noticed early on that certain things were just... slower with ViewCommands. It was definitely noticeable, but we determined it to be acceptable and moved on.

Recently, it became clear to me that the perf regression may not be acceptable, but there might be an "easy" solution.

# Why are ViewCommands slower than SetNativeProps?

So, a couple things. SetNativeProps on Paper would actually cause a layout pass; the same is not true for ViewCommands, so they should actually be much faster. But they're not! The reason is that ViewCommands are treated as regular mount items, and they are queued up /after/ all other mount items. That means if you're trying to interact with the UI while some part of it is updating, your ViewCommands must wait for portions of the screen to finish rendering before they execute.

In some cases, those views may even disappear before the ViewCommand executes, causing increased exceptions as well as speed degredations.

# Proposal

This experiment that ran with successful results was: to execute ViewCommands /before/ all other types of mount instructions (by maintaining a separate queue). That means if you tap on a TextInput to focus it while the screen is doing some heavy rendering, the next time the UIManager executes a batch of instructions, it will execute the focus operation out-of-order, at the very beginning. From a user perspective this is actually quite noticeable, and works much better than the older behavior.

# Why it's Not That Dangerous

* Is it possible that we'll execute instructions after the corresponding view has disappeared? This was already possible, and is actually less likely now, since it's more likely that the ViewCommand will execute before the Delete instructions executes.
* Is it possible we'll execute instructions BEFORE the view is created? Yes, this is possible and I actually found a repro for it. My solution: allow ViewCommands to be retried, exactly once. If they throw an exception the first time, we requeue which will cause the command to be executed after the current batch of mount items. Interestingly, this seems to be unnecessary in Fabric, so the logic there is a bit simpler (probably because on Android we do view preallocation under Fabric, so views are created way before they're inserted into the view hierarchy, and apparently before ViewCommands have a chance to execute).
* ViewCommands are already an imperative feature that exists outside of the normal React commit cycle. So they're already dangerous. This doesn't change that, but it does make dangerous code *faster*, so that's good.

Changelog: [Android][Changed] ViewCommands on Android now execute earlier, as a perf optimization.

Reviewed By: mdvacca

Differential Revision: D22343648

fbshipit-source-id: 310d94977ac8ca3140ee8aa272272f660efafa36
  • Loading branch information
JoshuaGross authored and facebook-github-bot committed Jul 2, 2020
1 parent a36d9cd commit c6b9cc3
Showing 1 changed file with 1 addition and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -866,8 +866,7 @@ public void run() {
long runStartTime = SystemClock.uptimeMillis();

// All ViewCommands should be executed first as a perf optimization.
// This entire block is only executed if there's a separate viewCommand queue,
// which is currently gated by a ReactFeatureFlag.
// This entire block is only executed if there's at least one ViewCommand queued.
if (viewCommandOperations != null) {
for (DispatchCommandViewOperation op : viewCommandOperations) {
try {
Expand Down

0 comments on commit c6b9cc3

Please sign in to comment.