From dc2df754267df3909631d81c22b9fcab58dfa241 Mon Sep 17 00:00:00 2001 From: Nico Burns Date: Wed, 29 Jul 2020 02:43:17 -0700 Subject: [PATCH 1/7] Make flipper pods configurations configurable (#29074) Summary: This PR allows the use_flipper! helper function to be used in projects that have more than just Debug/Release configurations. For example, a project may have DevDebug, DevRelease, ProdDebug, and ProdRelease configurations. These projects can now do: ```ruby use_flipper!(configurations: ['DevDebug', 'ProdDebug']) ``` ## Changelog [iOS][Added] Ability to set which configuration to enable flipper for when using use_flipper! Pull Request resolved: https://github.com/facebook/react-native/pull/29074 Test Plan: I don't know how to run code in this repository, so I copy and pasted this function into the Podfile of an existing project. My complete Podfile is as below: ```ruby # Uncomment the next line to define a global platform for your project platform :ios, '10.0' require_relative '../node_modules/react-native-community/cli-platform-ios/native_modules' project 'marketplace', 'Dev.Debug' => :debug, 'Dev.Release' => :release, 'Prod.Debug' => :debug, 'Prod.Release' => :release def use_flipper!(versions = {}, configurations: ['Debug']) versions['Flipper'] ||= '~> 0.33.1' versions['DoubleConversion'] ||= '1.1.7' versions['Flipper-Folly'] ||= '~> 2.1' versions['Flipper-Glog'] ||= '0.3.6' versions['Flipper-PeerTalk'] ||= '~> 0.0.4' versions['Flipper-RSocket'] ||= '~> 1.0' pod 'FlipperKit', versions['Flipper'], :configurations => configurations pod 'FlipperKit/FlipperKitLayoutPlugin', versions['Flipper'], :configurations => configurations pod 'FlipperKit/SKIOSNetworkPlugin', versions['Flipper'], :configurations => configurations pod 'FlipperKit/FlipperKitUserDefaultsPlugin', versions['Flipper'], :configurations => configurations pod 'FlipperKit/FlipperKitReactPlugin', versions['Flipper'], :configurations => configurations # List all transitive dependencies for FlipperKit pods # to avoid them being linked in Release builds pod 'Flipper', versions['Flipper'], :configurations => configurations pod 'Flipper-DoubleConversion', versions['Flipper-DoubleConversion'], :configurations => configurations pod 'Flipper-Folly', versions['Flipper-Folly'], :configurations => configurations pod 'Flipper-Glog', versions['Flipper-Glog'], :configurations => configurations pod 'Flipper-PeerTalk', versions['Flipper-PeerTalk'], :configurations => configurations pod 'Flipper-RSocket', versions['Flipper-RSocket'], :configurations => configurations pod 'FlipperKit/Core', versions['Flipper'], :configurations => configurations pod 'FlipperKit/CppBridge', versions['Flipper'], :configurations => configurations pod 'FlipperKit/FBCxxFollyDynamicConvert', versions['Flipper'], :configurations => configurations pod 'FlipperKit/FBDefines', versions['Flipper'], :configurations => configurations pod 'FlipperKit/FKPortForwarding', versions['Flipper'], :configurations => configurations pod 'FlipperKit/FlipperKitHighlightOverlay', versions['Flipper'], :configurations => configurations pod 'FlipperKit/FlipperKitLayoutTextSearchable', versions['Flipper'], :configurations => configurations pod 'FlipperKit/FlipperKitNetworkPlugin', versions['Flipper'], :configurations => configurations end # Post Install processing for Flipper def flipper_post_install(installer) installer.pods_project.targets.each do |target| if target.name == 'YogaKit' target.build_configurations.each do |config| config.build_settings['SWIFT_VERSION'] = '4.1' end end end end target 'marketplace' do # Comment the next line if you're not using Swift and don't want to use dynamic frameworks #use_frameworks! pod 'FBLazyVector', :path => '../node_modules/react-native/Libraries/FBLazyVector' pod 'FBReactNativeSpec', :path => '../node_modules/react-native/Libraries/FBReactNativeSpec' pod 'RCTRequired', :path => '../node_modules/react-native/Libraries/RCTRequired' pod 'RCTTypeSafety', :path => '../node_modules/react-native/Libraries/TypeSafety' pod 'React', :path => '../node_modules/react-native/' pod 'React-Core', :path => '../node_modules/react-native/' pod 'React-CoreModules', :path => '../node_modules/react-native/React/CoreModules' pod 'React-Core/DevSupport', :path => '../node_modules/react-native/' pod 'React-RCTActionSheet', :path => '../node_modules/react-native/Libraries/ActionSheetIOS' pod 'React-RCTAnimation', :path => '../node_modules/react-native/Libraries/NativeAnimation' pod 'React-RCTBlob', :path => '../node_modules/react-native/Libraries/Blob' pod 'React-RCTImage', :path => '../node_modules/react-native/Libraries/Image' pod 'React-RCTLinking', :path => '../node_modules/react-native/Libraries/LinkingIOS' pod 'React-RCTNetwork', :path => '../node_modules/react-native/Libraries/Network' pod 'React-RCTSettings', :path => '../node_modules/react-native/Libraries/Settings' pod 'React-RCTText', :path => '../node_modules/react-native/Libraries/Text' pod 'React-RCTVibration', :path => '../node_modules/react-native/Libraries/Vibration' pod 'React-Core/RCTWebSocket', :path => '../node_modules/react-native/' pod 'React-cxxreact', :path => '../node_modules/react-native/ReactCommon/cxxreact' pod 'React-jsi', :path => '../node_modules/react-native/ReactCommon/jsi' pod 'React-jsiexecutor', :path => '../node_modules/react-native/ReactCommon/jsiexecutor' pod 'React-jsinspector', :path => '../node_modules/react-native/ReactCommon/jsinspector' pod 'ReactCommon/callinvoker', :path => '../node_modules/react-native/ReactCommon' pod 'ReactCommon/turbomodule/core', :path => '../node_modules/react-native/ReactCommon' pod 'Yoga', :path => '../node_modules/react-native/ReactCommon/yoga', :modular_headers => true pod 'DoubleConversion', :podspec => '../node_modules/react-native/third-party-podspecs/DoubleConversion.podspec' pod 'glog', :podspec => '../node_modules/react-native/third-party-podspecs/glog.podspec' pod 'Folly', :podspec => '../node_modules/react-native/third-party-podspecs/Folly.podspec' pod 'Intercom', '~> 6.0.0' pod 'RNDeviceInfo', :path => '../node_modules/react-native-device-info' pod 'react-native-netinfo', :podspec => '../node_modules/react-native-community/netinfo/react-native-netinfo.podspec' pod 'BugsnagReactNative', :podspec => '../node_modules/bugsnag-react-native/BugsnagReactNative.podspec' pod 'rn-fetch-blob', :podspec => '../node_modules/rn-fetch-blob/rn-fetch-blob.podspec' use_native_modules! pod 'react-native-intercom', :path => '../node_modules/react-native-intercom' pod 'react-native-image-resizer', :path => '../node_modules/react-native-image-resizer' pod 'RNVectorIcons', :path => '../node_modules/react-native-vector-icons' pod 'react-native-camera', :path => '../node_modules/react-native-camera' pod 'RNCPushNotificationIOS', :path => '../node_modules/react-native-community/push-notification-ios' pod 'RNDateTimePicker', :path => '../node_modules/react-native-community/datetimepicker' # Enables Flipper. # # Note that if you have use_frameworks! enabled, Flipper will not work and # you should disable these next few lines. use_flipper!(configurations: ['Dev.Debug', 'Prod.Debug']) post_install do |installer| flipper_post_install(installer) installer.pods_project.targets.each do |target| # The following is needed to ensure the "archive" step works in XCode. # It removes React & Yoga from the Pods project, as it is already included in the main project. # Without this, you'd see errors when you archive like: # "Multiple commands produce ... libReact.a" # "Multiple commands produce ... libyoga.a" targets_to_ignore = %w(React yoga) if targets_to_ignore.include? target.name target.remove_from_project end end end end ``` I then ran `pod install`: ``` nico:ios/ (dev*) $ pod install [20:14:43] Adding a custom script phase for Pod RNFBApp: [RNFB] Core Configuration Detected React Native module pods for BVLinearGradient, RNCAsyncStorage, RNCPicker, RNCPushNotificationIOS, RNDateTimePicker, RNFBApp, RNFBAuth, RNFBDatabase, RNFBDynamicLinks, RNFBFirestore, RNFBStorage, RNSVG, RNVectorIcons, ReactNativeNavigation, react-native-camera, react-native-cameraroll, react-native-config, react-native-flipper, react-native-get-random-values, react-native-image-resizer, and react-native-intercom Analyzing dependencies Downloading dependencies Generating Pods project Integrating client project Pod installation complete! There are 73 dependencies from the Podfile and 88 total pods installed. [!] use_native_modules! skipped the react-native dependency 'react-native-photo-view'. No podspec file was found. - Check to see if there is an updated version that contains the necessary podspec file - Contact the library maintainers or send them a PR to add a podspec. The react-native-webview podspec is a good example of a package.json driven podspec. See https://github.com/react-native-community/react-native-webview/blob/master/react-native-webview.podspec - If necessary, you can disable autolinking for the dependency and link it manually. See https://github.com/react-native-community/cli/blob/master/docs/autolinking.md#how-can-i-disable-autolinking-for-unsupported-library [!] use_native_modules! skipped the react-native dependency 'detox'. No podspec file was found. - Check to see if there is an updated version that contains the necessary podspec file - Contact the library maintainers or send them a PR to add a podspec. The react-native-webview podspec is a good example of a package.json driven podspec. See https://github.com/react-native-community/react-native-webview/blob/master/react-native-webview.podspec - If necessary, you can disable autolinking for the dependency and link it manually. See https://github.com/react-native-community/cli/blob/master/docs/autolinking.md#how-can-i-disable-autolinking-for-unsupported-library ``` Reviewed By: cpojer Differential Revision: D22795421 Pulled By: passy fbshipit-source-id: 89ba555eadc4918e9ac464a19a318198b237e01e --- scripts/react_native_pods.rb | 40 ++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/scripts/react_native_pods.rb b/scripts/react_native_pods.rb index 8b04aaf8ba..73bb9ad55a 100644 --- a/scripts/react_native_pods.rb +++ b/scripts/react_native_pods.rb @@ -59,34 +59,34 @@ def use_react_native! (options={}) end end -def use_flipper!(versions = {}) +def use_flipper!(versions = {}, configurations: ['Debug']) versions['Flipper'] ||= '~> 0.41.1' versions['Flipper-DoubleConversion'] ||= '1.1.7' versions['Flipper-Folly'] ||= '~> 2.2' versions['Flipper-Glog'] ||= '0.3.6' versions['Flipper-PeerTalk'] ||= '~> 0.0.4' versions['Flipper-RSocket'] ||= '~> 1.1' - pod 'FlipperKit', versions['Flipper'], :configuration => 'Debug' - pod 'FlipperKit/FlipperKitLayoutPlugin', versions['Flipper'], :configuration => 'Debug' - pod 'FlipperKit/SKIOSNetworkPlugin', versions['Flipper'], :configuration => 'Debug' - pod 'FlipperKit/FlipperKitUserDefaultsPlugin', versions['Flipper'], :configuration => 'Debug' - pod 'FlipperKit/FlipperKitReactPlugin', versions['Flipper'], :configuration => 'Debug' + pod 'FlipperKit', versions['Flipper'], :configurations => configurations + pod 'FlipperKit/FlipperKitLayoutPlugin', versions['Flipper'], :configurations => configurations + pod 'FlipperKit/SKIOSNetworkPlugin', versions['Flipper'], :configurations => configurations + pod 'FlipperKit/FlipperKitUserDefaultsPlugin', versions['Flipper'], :configurations => configurations + pod 'FlipperKit/FlipperKitReactPlugin', versions['Flipper'], :configurations => configurations # List all transitive dependencies for FlipperKit pods # to avoid them being linked in Release builds - pod 'Flipper', versions['Flipper'], :configuration => 'Debug' - pod 'Flipper-DoubleConversion', versions['Flipper-DoubleConversion'], :configuration => 'Debug' - pod 'Flipper-Folly', versions['Flipper-Folly'], :configuration => 'Debug' - pod 'Flipper-Glog', versions['Flipper-Glog'], :configuration => 'Debug' - pod 'Flipper-PeerTalk', versions['Flipper-PeerTalk'], :configuration => 'Debug' - pod 'Flipper-RSocket', versions['Flipper-RSocket'], :configuration => 'Debug' - pod 'FlipperKit/Core', versions['Flipper'], :configuration => 'Debug' - pod 'FlipperKit/CppBridge', versions['Flipper'], :configuration => 'Debug' - pod 'FlipperKit/FBCxxFollyDynamicConvert', versions['Flipper'], :configuration => 'Debug' - pod 'FlipperKit/FBDefines', versions['Flipper'], :configuration => 'Debug' - pod 'FlipperKit/FKPortForwarding', versions['Flipper'], :configuration => 'Debug' - pod 'FlipperKit/FlipperKitHighlightOverlay', versions['Flipper'], :configuration => 'Debug' - pod 'FlipperKit/FlipperKitLayoutTextSearchable', versions['Flipper'], :configuration => 'Debug' - pod 'FlipperKit/FlipperKitNetworkPlugin', versions['Flipper'], :configuration => 'Debug' + pod 'Flipper', versions['Flipper'], :configurations => configurations + pod 'Flipper-DoubleConversion', versions['Flipper-DoubleConversion'], :configurations => configurations + pod 'Flipper-Folly', versions['Flipper-Folly'], :configurations => configurations + pod 'Flipper-Glog', versions['Flipper-Glog'], :configurations => configurations + pod 'Flipper-PeerTalk', versions['Flipper-PeerTalk'], :configurations => configurations + pod 'Flipper-RSocket', versions['Flipper-RSocket'], :configurations => configurations + pod 'FlipperKit/Core', versions['Flipper'], :configurations => configurations + pod 'FlipperKit/CppBridge', versions['Flipper'], :configurations => configurations + pod 'FlipperKit/FBCxxFollyDynamicConvert', versions['Flipper'], :configurations => configurations + pod 'FlipperKit/FBDefines', versions['Flipper'], :configurations => configurations + pod 'FlipperKit/FKPortForwarding', versions['Flipper'], :configurations => configurations + pod 'FlipperKit/FlipperKitHighlightOverlay', versions['Flipper'], :configurations => configurations + pod 'FlipperKit/FlipperKitLayoutTextSearchable', versions['Flipper'], :configurations => configurations + pod 'FlipperKit/FlipperKitNetworkPlugin', versions['Flipper'], :configurations => configurations end # Post Install processing for Flipper From 9e6ba9ddb8608d4e3a4a80d0138600130b766d4c Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Wed, 29 Jul 2020 12:37:11 -0700 Subject: [PATCH 2/7] Animated: queue all NativeAnimated operations until mounting effects are finished Summary: In Fabric, there are two subtle differences in Animated: 1) On the native side, queued NativeAnimated operations are executed more frequently 2) On the JS side there seems to be a bigger gap between AnimatedNodes being set up, and the final `componentDidMount`/`componentDidUpdate` that results in `_attachNativeEvents` being called. This results in some visual glitching for certain features like StickyScrollViewHeader and other animations that are frequently rerendered. Rerendering an animated component relies on sync-like behavior (new animations must be set up in the same frame that old animations are torn down, and the UI must flush any updates in the same frame). This sync-like behavior is trickier to achieve in Fabric, so we introduce this mechanism, in Fabric only, to queued operations more aggressively on the JS side and only flush them when `componentDidMount`/`componentDidUpdate` have been called on all relevant nodes. This seems to resolve longstanding issues with animations in Fabric on Android. There are unrelated issues on iOS that make this difficult to test currently, so I'll keep the new path Android-only for now. Changelog: [Internal] Reviewed By: sammy-SC Differential Revision: D22801173 fbshipit-source-id: 3db2235483636c9074df941cfac0a30f3f97545a --- .../Animated/src/NativeAnimatedHelper.js | 130 +++++++++++++----- .../Animated/src/createAnimatedComponent.js | 82 ++++++++--- 2 files changed, 158 insertions(+), 54 deletions(-) diff --git a/Libraries/Animated/src/NativeAnimatedHelper.js b/Libraries/Animated/src/NativeAnimatedHelper.js index ed2236a859..5b4ac69eab 100644 --- a/Libraries/Animated/src/NativeAnimatedHelper.js +++ b/Libraries/Animated/src/NativeAnimatedHelper.js @@ -35,8 +35,10 @@ let __nativeAnimationIdCount = 1; /* used for started animations */ let nativeEventEmitter; +let waitingForQueuedOperations = new Set(); +let queueOperations = false; let queueConnections = false; -let queue = []; +let queue: Array<() => void> = []; /** * Simple wrappers around NativeAnimatedModule to provide flow and autocomplete support for @@ -55,38 +57,74 @@ const API = { NativeAnimatedModule.getValue(tag, saveValueCallback); } }, + setWaitingForIdentifier: function(id: number): void { + invariant(NativeAnimatedModule, 'Native animated module is not available'); + waitingForQueuedOperations.add(id); + queueOperations = true; + queueConnections = true; + }, + unsetWaitingForIdentifier: function(id: number): void { + invariant(NativeAnimatedModule, 'Native animated module is not available'); + waitingForQueuedOperations.delete(id); + + if (waitingForQueuedOperations.size === 0) { + queueOperations = false; + API.disableQueue(); + } + }, disableQueue: function(): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); queueConnections = false; - for (let q = 0, l = queue.length; q < l; q++) { - const args = queue[q]; - NativeAnimatedModule.connectAnimatedNodes(args[0], args[1]); + if (!queueOperations) { + for (let q = 0, l = queue.length; q < l; q++) { + queue[q](); + } + queue.length = 0; + } + }, + queueConnection: (fn: () => void): void => { + if (queueConnections) { + queue.push(fn); + } else { + fn(); + } + }, + queueOperation: (fn: () => void): void => { + if (queueOperations) { + queue.push(fn); + } else { + fn(); } - queue.length = 0; }, createAnimatedNode: function(tag: number, config: AnimatedNodeConfig): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.createAnimatedNode(tag, config); + API.queueOperation(() => + NativeAnimatedModule.createAnimatedNode(tag, config), + ); }, startListeningToAnimatedNodeValue: function(tag: number) { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.startListeningToAnimatedNodeValue(tag); + API.queueOperation(() => + NativeAnimatedModule.startListeningToAnimatedNodeValue(tag), + ); }, stopListeningToAnimatedNodeValue: function(tag: number) { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.stopListeningToAnimatedNodeValue(tag); + API.queueOperation(() => + NativeAnimatedModule.stopListeningToAnimatedNodeValue(tag), + ); }, connectAnimatedNodes: function(parentTag: number, childTag: number): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); - if (queueConnections) { - queue.push([parentTag, childTag]); - return; - } - NativeAnimatedModule.connectAnimatedNodes(parentTag, childTag); + API.queueConnection(() => + NativeAnimatedModule.connectAnimatedNodes(parentTag, childTag), + ); }, disconnectAnimatedNodes: function(parentTag: number, childTag: number): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.disconnectAnimatedNodes(parentTag, childTag); + API.queueOperation(() => + NativeAnimatedModule.disconnectAnimatedNodes(parentTag, childTag), + ); }, startAnimatingNode: function( animationId: number, @@ -95,54 +133,70 @@ const API = { endCallback: EndCallback, ): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.startAnimatingNode( - animationId, - nodeTag, - config, - endCallback, + API.queueOperation(() => + NativeAnimatedModule.startAnimatingNode( + animationId, + nodeTag, + config, + endCallback, + ), ); }, stopAnimation: function(animationId: number) { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.stopAnimation(animationId); + API.queueOperation(() => NativeAnimatedModule.stopAnimation(animationId)); }, setAnimatedNodeValue: function(nodeTag: number, value: number): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.setAnimatedNodeValue(nodeTag, value); + API.queueOperation(() => + NativeAnimatedModule.setAnimatedNodeValue(nodeTag, value), + ); }, setAnimatedNodeOffset: function(nodeTag: number, offset: number): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.setAnimatedNodeOffset(nodeTag, offset); + API.queueOperation(() => + NativeAnimatedModule.setAnimatedNodeOffset(nodeTag, offset), + ); }, flattenAnimatedNodeOffset: function(nodeTag: number): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.flattenAnimatedNodeOffset(nodeTag); + API.queueOperation(() => + NativeAnimatedModule.flattenAnimatedNodeOffset(nodeTag), + ); }, extractAnimatedNodeOffset: function(nodeTag: number): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.extractAnimatedNodeOffset(nodeTag); + API.queueOperation(() => + NativeAnimatedModule.extractAnimatedNodeOffset(nodeTag), + ); }, connectAnimatedNodeToView: function(nodeTag: number, viewTag: number): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.connectAnimatedNodeToView(nodeTag, viewTag); + API.queueOperation(() => + NativeAnimatedModule.connectAnimatedNodeToView(nodeTag, viewTag), + ); }, disconnectAnimatedNodeFromView: function( nodeTag: number, viewTag: number, ): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.disconnectAnimatedNodeFromView(nodeTag, viewTag); + API.queueOperation(() => + NativeAnimatedModule.disconnectAnimatedNodeFromView(nodeTag, viewTag), + ); }, restoreDefaultValues: function(nodeTag: number): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); // Backwards compat with older native runtimes, can be removed later. if (NativeAnimatedModule.restoreDefaultValues != null) { - NativeAnimatedModule.restoreDefaultValues(nodeTag); + API.queueOperation(() => + NativeAnimatedModule.restoreDefaultValues(nodeTag), + ); } }, dropAnimatedNode: function(tag: number): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.dropAnimatedNode(tag); + API.queueOperation(() => NativeAnimatedModule.dropAnimatedNode(tag)); }, addAnimatedEventToView: function( viewTag: number, @@ -150,10 +204,12 @@ const API = { eventMapping: EventMapping, ) { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.addAnimatedEventToView( - viewTag, - eventName, - eventMapping, + API.queueOperation(() => + NativeAnimatedModule.addAnimatedEventToView( + viewTag, + eventName, + eventMapping, + ), ); }, removeAnimatedEventFromView( @@ -162,10 +218,12 @@ const API = { animatedNodeTag: number, ) { invariant(NativeAnimatedModule, 'Native animated module is not available'); - NativeAnimatedModule.removeAnimatedEventFromView( - viewTag, - eventName, - animatedNodeTag, + API.queueOperation(() => + NativeAnimatedModule.removeAnimatedEventFromView( + viewTag, + eventName, + animatedNodeTag, + ), ); }, }; diff --git a/Libraries/Animated/src/createAnimatedComponent.js b/Libraries/Animated/src/createAnimatedComponent.js index 4851399fd1..22d0a2f62b 100644 --- a/Libraries/Animated/src/createAnimatedComponent.js +++ b/Libraries/Animated/src/createAnimatedComponent.js @@ -11,13 +11,17 @@ 'use strict'; const View = require('../../Components/View/View'); +const Platform = require('../../Utilities/Platform'); const {AnimatedEvent} = require('./AnimatedEvent'); const AnimatedProps = require('./nodes/AnimatedProps'); const React = require('react'); +const NativeAnimatedHelper = require('./NativeAnimatedHelper'); const invariant = require('invariant'); const setAndForwardRef = require('../../Utilities/setAndForwardRef'); +let animatedComponentNextId = 1; + export type AnimatedComponentType< Props: {+[string]: mixed, ...}, Instance, @@ -51,6 +55,9 @@ function createAnimatedComponent( _propsAnimated: AnimatedProps; _eventDetachers: Array = []; + // Only to be used in this file, and only in Fabric. + _animatedComponentId: number = -1; + _attachNativeEvents() { // Make sure to get the scrollable node for components that implement // `ScrollResponder.Mixin`. @@ -72,25 +79,11 @@ function createAnimatedComponent( this._eventDetachers = []; } - // The system is best designed when setNativeProps is implemented. It is - // able to avoid re-rendering and directly set the attributes that changed. - // However, setNativeProps can only be implemented on leaf native - // components. If you want to animate a composite component, you need to - // re-render it. In this case, we have a fallback that uses forceUpdate. - // This fallback is also called in Fabric. - _animatedPropsCallback = () => { + _isFabric = (): boolean => { if (this._component == null) { - // AnimatedProps is created in will-mount because it's used in render. - // But this callback may be invoked before mount in async mode, - // In which case we should defer the setNativeProps() call. - // React may throw away uncommitted work in async mode, - // So a deferred call won't always be invoked. - this._invokeAnimatedPropsCallbackOnMount = true; - } else if ( - process.env.NODE_ENV === 'test' || - // For animating properties of non-leaf/non-native components - typeof this._component.setNativeProps !== 'function' || - // In Fabric, force animations to go through forceUpdate and skip setNativeProps + return false; + } + return ( // eslint-disable-next-line dot-notation this._component['_internalInstanceHandle']?.stateNode?.canonical != null || @@ -114,6 +107,54 @@ function createAnimatedComponent( // eslint-disable-next-line dot-notation '_internalInstanceHandle' ]?.stateNode?.canonical != null) + ); + }; + + _waitForUpdate = (): void => { + // If this works well on iOS, we should remove this check + if (Platform.OS === 'android') { + if (this._isFabric()) { + if (this._animatedComponentId === -1) { + this._animatedComponentId = animatedComponentNextId++; + } + NativeAnimatedHelper.API.setWaitingForIdentifier( + this._animatedComponentId, + ); + } + } + }; + + _markUpdateComplete = (): void => { + // If this works well on iOS, we should remove this check + if (Platform.OS === 'android') { + if (this._isFabric()) { + NativeAnimatedHelper.API.unsetWaitingForIdentifier( + this._animatedComponentId, + ); + } + } + }; + + // The system is best designed when setNativeProps is implemented. It is + // able to avoid re-rendering and directly set the attributes that changed. + // However, setNativeProps can only be implemented on leaf native + // components. If you want to animate a composite component, you need to + // re-render it. In this case, we have a fallback that uses forceUpdate. + // This fallback is also called in Fabric. + _animatedPropsCallback = () => { + if (this._component == null) { + // AnimatedProps is created in will-mount because it's used in render. + // But this callback may be invoked before mount in async mode, + // In which case we should defer the setNativeProps() call. + // React may throw away uncommitted work in async mode, + // So a deferred call won't always be invoked. + this._invokeAnimatedPropsCallbackOnMount = true; + } else if ( + process.env.NODE_ENV === 'test' || + // For animating properties of non-leaf/non-native components + typeof this._component.setNativeProps !== 'function' || + // In Fabric, force animations to go through forceUpdate and skip setNativeProps + this._isFabric() ) { this.forceUpdate(); } else if (!this._propsAnimated.__isNative) { @@ -199,6 +240,7 @@ function createAnimatedComponent( } UNSAFE_componentWillMount() { + this._waitForUpdate(); this._attachProps(this.props); } @@ -210,9 +252,11 @@ function createAnimatedComponent( this._propsAnimated.setNativeView(this._component); this._attachNativeEvents(); + this._markUpdateComplete(); } UNSAFE_componentWillReceiveProps(newProps) { + this._waitForUpdate(); this._attachProps(newProps); } @@ -224,11 +268,13 @@ function createAnimatedComponent( this._detachNativeEvents(); this._attachNativeEvents(); } + this._markUpdateComplete(); } componentWillUnmount() { this._propsAnimated && this._propsAnimated.__detach(); this._detachNativeEvents(); + this._markUpdateComplete(); } } From a1ed73dd6436cd12cae17f7bbd361a678557426d Mon Sep 17 00:00:00 2001 From: Jiawei Lv Date: Wed, 29 Jul 2020 15:09:05 -0700 Subject: [PATCH 3/7] Make some RN rules no-op for arc focus (#29510) Summary: ## Changelog: [Internal] ## Test Plan: run buck project Reviewed By: cute-jumper Differential Revision: D22495239 fbshipit-source-id: b77c65632ee5fb0efd0d5980f11c48d76a4fdbb6 --- packages/react-native-codegen/DEFS.bzl | 219 +++++++++++++------------ 1 file changed, 116 insertions(+), 103 deletions(-) diff --git a/packages/react-native-codegen/DEFS.bzl b/packages/react-native-codegen/DEFS.bzl index b214f686a8..ade9c17940 100644 --- a/packages/react-native-codegen/DEFS.bzl +++ b/packages/react-native-codegen/DEFS.bzl @@ -1,3 +1,4 @@ +load("@fbsource//tools/build_defs:buckconfig.bzl", "read_bool") load("@fbsource//tools/build_defs:fb_native_wrapper.bzl", "fb_native") load("@fbsource//tools/build_defs:platform_defs.bzl", "IOS", "MACOSX") load("@fbsource//tools/build_defs/apple:flag_defs.bzl", "get_preprocessor_flags_for_build_mode") @@ -184,78 +185,84 @@ def rn_codegen_components( ) # libs - rn_xplat_cxx_library( - name = "generated_components-{}".format(name), - srcs = [ - ":{}".format(generate_event_emitter_cpp_name), - ":{}".format(generate_props_cpp_name), - ":{}".format(generate_shadow_node_cpp_name), - ], - headers = [ - ":{}".format(generate_component_descriptor_h_name), - ":{}".format(generate_event_emitter_h_name), - ":{}".format(generate_props_h_name), - ":{}".format(generate_shadow_node_h_name), - ], - header_namespace = "react/components/{}".format(name), - exported_headers = { - "ComponentDescriptors.h": ":{}".format(generate_component_descriptor_h_name), - "EventEmitters.h": ":{}".format(generate_event_emitter_h_name), - "Props.h": ":{}".format(generate_props_h_name), - "RCTComponentViewHelpers.h": ":{}".format(generate_component_hobjcpp_name), - "ShadowNodes.h": ":{}".format(generate_shadow_node_h_name), - }, - compiler_flags = [ - "-fexceptions", - "-frtti", - "-std=c++14", - "-Wall", - ], - fbobjc_compiler_flags = get_apple_compiler_flags(), - fbobjc_preprocessor_flags = get_preprocessor_flags_for_build_mode() + get_apple_inspector_flags(), - ios_exported_headers = { - "ComponentViewHelpers.h": ":{}".format(generate_component_hobjcpp_name), - }, - ios_headers = [ - ":{}".format(generate_component_hobjcpp_name), - ], - labels = ["codegen_rule"], - platforms = (ANDROID, APPLE, CXX), - preprocessor_flags = [ - "-DLOG_TAG=\"ReactNative\"", - "-DWITH_FBSYSTRACE=1", - ], - tests = [":generated_tests-{}".format(name)], - visibility = ["PUBLIC"], - deps = [ - "//third-party/glog:glog", - "//xplat/fbsystrace:fbsystrace", - "//xplat/folly:headers_only", - "//xplat/folly:memory", - "//xplat/folly:molly", - YOGA_CXX_TARGET, - react_native_xplat_target("fabric/debug:debug"), - react_native_xplat_target("fabric/core:core"), - react_native_xplat_target("fabric/graphics:graphics"), - react_native_xplat_target("fabric/components/image:image"), - react_native_xplat_target("fabric/imagemanager:imagemanager"), - react_native_xplat_target("fabric/components/view:view"), - ], - ) + if is_running_buck_project(): + rn_xplat_cxx_library(name = "generated_components-{}".format(name), visibility = ["PUBLIC"]) + else: + rn_xplat_cxx_library( + name = "generated_components-{}".format(name), + srcs = [ + ":{}".format(generate_event_emitter_cpp_name), + ":{}".format(generate_props_cpp_name), + ":{}".format(generate_shadow_node_cpp_name), + ], + headers = [ + ":{}".format(generate_component_descriptor_h_name), + ":{}".format(generate_event_emitter_h_name), + ":{}".format(generate_props_h_name), + ":{}".format(generate_shadow_node_h_name), + ], + header_namespace = "react/components/{}".format(name), + exported_headers = { + "ComponentDescriptors.h": ":{}".format(generate_component_descriptor_h_name), + "EventEmitters.h": ":{}".format(generate_event_emitter_h_name), + "Props.h": ":{}".format(generate_props_h_name), + "RCTComponentViewHelpers.h": ":{}".format(generate_component_hobjcpp_name), + "ShadowNodes.h": ":{}".format(generate_shadow_node_h_name), + }, + compiler_flags = [ + "-fexceptions", + "-frtti", + "-std=c++14", + "-Wall", + ], + fbobjc_compiler_flags = get_apple_compiler_flags(), + fbobjc_preprocessor_flags = get_preprocessor_flags_for_build_mode() + get_apple_inspector_flags(), + ios_exported_headers = { + "ComponentViewHelpers.h": ":{}".format(generate_component_hobjcpp_name), + }, + ios_headers = [ + ":{}".format(generate_component_hobjcpp_name), + ], + labels = ["codegen_rule"], + platforms = (ANDROID, APPLE, CXX), + preprocessor_flags = [ + "-DLOG_TAG=\"ReactNative\"", + "-DWITH_FBSYSTRACE=1", + ], + tests = [":generated_tests-{}".format(name)], + visibility = ["PUBLIC"], + deps = [ + "//third-party/glog:glog", + "//xplat/fbsystrace:fbsystrace", + "//xplat/folly:headers_only", + "//xplat/folly:memory", + "//xplat/folly:molly", + YOGA_CXX_TARGET, + react_native_xplat_target("fabric/debug:debug"), + react_native_xplat_target("fabric/core:core"), + react_native_xplat_target("fabric/graphics:graphics"), + react_native_xplat_target("fabric/components/image:image"), + react_native_xplat_target("fabric/imagemanager:imagemanager"), + react_native_xplat_target("fabric/components/view:view"), + ], + ) - rn_android_library( - name = "generated_components_java-{}".format(name), - srcs = [ - ":{}".format(zip_generated_java_files), - ], - labels = ["codegen_rule"], - visibility = ["PUBLIC"], - deps = [ - react_native_dep("third-party/android/androidx:annotation"), - react_native_target("java/com/facebook/react/bridge:bridge"), - react_native_target("java/com/facebook/react/uimanager:uimanager"), - ], - ) + if is_running_buck_project(): + rn_android_library(name = "generated_components_java-{}".format(name)) + else: + rn_android_library( + name = "generated_components_java-{}".format(name), + srcs = [ + ":{}".format(zip_generated_java_files), + ], + labels = ["codegen_rule"], + visibility = ["PUBLIC"], + deps = [ + react_native_dep("third-party/android/androidx:annotation"), + react_native_target("java/com/facebook/react/bridge:bridge"), + react_native_target("java/com/facebook/react/uimanager:uimanager"), + ], + ) # Tests fb_xplat_cxx_test( @@ -308,35 +315,41 @@ def rn_codegen_cxx_modules( labels = ["codegen_rule"], ) - rn_xplat_cxx_library( - name = "generated_cxx_modules-{}".format(name), - srcs = [ - ":{}".format(generate_module_cpp_name), - ], - headers = [ - ":{}".format(generate_module_h_name), - ], - header_namespace = "react/modules/{}".format(name), - exported_headers = { - "NativeModules.cpp": ":{}".format(generate_module_cpp_name), - "NativeModules.h": ":{}".format(generate_module_h_name), - }, - compiler_flags = [ - "-fexceptions", - "-frtti", - "-std=c++14", - "-Wall", - ], - fbobjc_compiler_flags = get_apple_compiler_flags(), - fbobjc_preprocessor_flags = get_preprocessor_flags_for_build_mode() + get_apple_inspector_flags(), - labels = ["codegen_rule"], - platforms = (ANDROID, APPLE), - preprocessor_flags = [ - "-DLOG_TAG=\"ReactNative\"", - "-DWITH_FBSYSTRACE=1", - ], - visibility = ["PUBLIC"], - exported_deps = [ - react_native_xplat_target("turbomodule/core:core"), - ], - ) + if is_running_buck_project(): + rn_xplat_cxx_library(name = "generated_cxx_modules-{}".format(name)) + else: + rn_xplat_cxx_library( + name = "generated_cxx_modules-{}".format(name), + srcs = [ + ":{}".format(generate_module_cpp_name), + ], + headers = [ + ":{}".format(generate_module_h_name), + ], + header_namespace = "react/modules/{}".format(name), + exported_headers = { + "NativeModules.cpp": ":{}".format(generate_module_cpp_name), + "NativeModules.h": ":{}".format(generate_module_h_name), + }, + compiler_flags = [ + "-fexceptions", + "-frtti", + "-std=c++14", + "-Wall", + ], + fbobjc_compiler_flags = get_apple_compiler_flags(), + fbobjc_preprocessor_flags = get_preprocessor_flags_for_build_mode() + get_apple_inspector_flags(), + labels = ["codegen_rule"], + platforms = (ANDROID, APPLE), + preprocessor_flags = [ + "-DLOG_TAG=\"ReactNative\"", + "-DWITH_FBSYSTRACE=1", + ], + visibility = ["PUBLIC"], + exported_deps = [ + react_native_xplat_target("turbomodule/core:core"), + ], + ) + +def is_running_buck_project(): + return read_bool("fbandroid", "is_running_buck_project", False) From 949dedca9ff2185b5cb39ce54c89f91b8df0a7ef Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Wed, 29 Jul 2020 15:38:31 -0700 Subject: [PATCH 4/7] Implement BackgroundExecutor for layouts Summary: As a followup to D22743723 (https://github.com/facebook/react-native/commit/d53fc8a3cd44c7ec7e6eade985daf3d4feb2d736) on the iOS side, I implement a BackgroundExecutor that can be used from C++ to schedule layouts on their own thread, off the JS and UI thread. This is a potential perf improvement that we will experiment with. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D22826795 fbshipit-source-id: 899bd67fe1b86f52910951e9536b59a1414a304c --- .../react/bridge/BackgroundExecutor.java | 46 +++++++++++++++++++ .../com/facebook/react/fabric/jni/Binding.cpp | 6 +++ .../com/facebook/react/fabric/jni/Binding.h | 2 + .../react/fabric/jni/JBackgroundExecutor.cpp | 35 ++++++++++++++ .../react/fabric/jni/JBackgroundExecutor.h | 34 ++++++++++++++ 5 files changed, 123 insertions(+) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/bridge/BackgroundExecutor.java create mode 100644 ReactAndroid/src/main/java/com/facebook/react/fabric/jni/JBackgroundExecutor.cpp create mode 100644 ReactAndroid/src/main/java/com/facebook/react/fabric/jni/JBackgroundExecutor.h diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/BackgroundExecutor.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/BackgroundExecutor.java new file mode 100644 index 0000000000..8b5d7c9425 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/BackgroundExecutor.java @@ -0,0 +1,46 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.bridge; + +import com.facebook.proguard.annotations.DoNotStrip; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +@DoNotStrip +public class BackgroundExecutor { + private static final String TAG = "FabricBackgroundExecutor"; + private final ExecutorService mExecutorService; + + @DoNotStrip + private BackgroundExecutor() { + mExecutorService = Executors.newFixedThreadPool(1); + } + + @DoNotStrip + private void queueRunnable(Runnable runnable) { + // Very rarely, an NPE is hit here - probably has to do with deallocation + // race conditions and the JNI. + // It's not clear yet which of these is most prevalent, or if either is a concern. + // If we don't find these logs in production then we can probably safely remove the logging, + // but it's also cheap to leave it here. + + if (runnable == null) { + ReactSoftException.logSoftException(TAG, new ReactNoCrashSoftException("runnable is null")); + return; + } + + final ExecutorService executorService = mExecutorService; + if (executorService == null) { + ReactSoftException.logSoftException( + TAG, new ReactNoCrashSoftException("executorService is null")); + return; + } + + executorService.execute(runnable); + } +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp index e44f1c232c..10e22094c1 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp @@ -278,6 +278,12 @@ void Binding::installFabricUIManager( toolbox.synchronousEventBeatFactory = synchronousBeatFactory; toolbox.asynchronousEventBeatFactory = asynchronousBeatFactory; + if (reactNativeConfig_->getBool( + "react_fabric:enable_background_executor_android")) { + backgroundExecutor_ = std::make_unique(); + toolbox.backgroundExecutor = backgroundExecutor_->get(); + } + if (enableLayoutAnimations) { animationDriver_ = std::make_shared(this); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h index b43f2b0532..5c2f33fad2 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h @@ -19,6 +19,7 @@ #include #include "ComponentFactoryDelegate.h" #include "EventBeatManager.h" +#include "JBackgroundExecutor.h" namespace facebook { namespace react { @@ -115,6 +116,7 @@ class Binding : public jni::HybridClass, virtual void onAllAnimationsComplete() override; LayoutAnimationDriver *getAnimationDriver(); std::shared_ptr animationDriver_; + std::unique_ptr backgroundExecutor_; std::shared_ptr scheduler_; std::mutex schedulerMutex_; diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/JBackgroundExecutor.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/JBackgroundExecutor.cpp new file mode 100644 index 0000000000..39cb4a4e59 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/JBackgroundExecutor.cpp @@ -0,0 +1,35 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#include +#include + +#include "JBackgroundExecutor.h" + +namespace facebook { +namespace react { + +using namespace facebook::jni; + +using facebook::react::JNativeRunnable; +using facebook::react::Runnable; + +BackgroundExecutor JBackgroundExecutor::get() { + auto self = JBackgroundExecutor::create(); + + return [self](std::function &&runnable) { + static auto method = + findClassStatic(JBackgroundExecutor::JBackgroundExecutorJavaDescriptor) + ->getMethod("queueRunnable"); + + auto jrunnable = JNativeRunnable::newObjectCxxArgs(std::move(runnable)); + method(self, static_ref_cast(jrunnable).get()); + }; +} + +} // namespace react +} // namespace facebook diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/JBackgroundExecutor.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/JBackgroundExecutor.h new file mode 100644 index 0000000000..002c0249d6 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/JBackgroundExecutor.h @@ -0,0 +1,34 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#pragma once + +#include +#include + +namespace facebook { +namespace react { + +using namespace facebook::jni; + +class JBackgroundExecutor : public JavaClass { + public: + static auto constexpr kJavaDescriptor = + "Lcom/facebook/react/bridge/BackgroundExecutor;"; + + constexpr static auto JBackgroundExecutorJavaDescriptor = + "com/facebook/react/bridge/BackgroundExecutor"; + + static global_ref create() { + return make_global(newInstance()); + } + + BackgroundExecutor get(); +}; + +} // namespace react +} // namespace facebook From 934561b2953848c730471a96524e51639ddd2dbd Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Wed, 29 Jul 2020 17:40:05 -0700 Subject: [PATCH 5/7] Remove complex NativeAnimated queueing mechanisms Summary: After D22801173 (https://github.com/facebook/react-native/commit/9e6ba9ddb8608d4e3a4a80d0138600130b766d4c) has landed, the native mechanisms in NativeAnimated to delay queued items from immediate execution are no longer necessary. Fabric and non-Fabric animations both look smooth after deleting this code. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D22807906 fbshipit-source-id: 9241fff84376f6aa9a35049cc40dfd6561effaa1 --- .../react/animated/NativeAnimatedModule.java | 74 ++----------------- .../react/bridge/UIManagerListener.java | 2 - .../react/fabric/FabricUIManager.java | 6 -- 3 files changed, 5 insertions(+), 77 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java index 249bf37758..dad43b29d7 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java @@ -88,16 +88,6 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec private abstract class UIThreadOperation { abstract void execute(NativeAnimatedNodesManager animatedNodesManager); - - long mFrameNumber = -1; - - public void setFrameNumber(long frameNumber) { - mFrameNumber = frameNumber; - } - - public long getFrameNumber() { - return mFrameNumber; - } } @NonNull private final GuardedFrameCallback mAnimatedFrameCallback; @@ -113,8 +103,6 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec private @Nullable NativeAnimatedNodesManager mNodesManager; - private volatile long mFrameNumber = 0; - private long mDispatchedFrameNumber = 0; private boolean mInitializedForFabric = false; private boolean mInitializedForNonFabric = false; private @UIManagerType int mUIManagerType = UIManagerType.DEFAULT; @@ -169,25 +157,13 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec } private void addOperation(UIThreadOperation operation) { - operation.setFrameNumber(mFrameNumber); mOperations.add(operation); } private void addPreOperation(UIThreadOperation operation) { - operation.setFrameNumber(mFrameNumber); mPreOperations.add(operation); } - // For FabricUIManager only - @Override - public void didScheduleMountItems(UIManager uiManager) { - if (mUIManagerType != UIManagerType.FABRIC) { - return; - } - - mFrameNumber++; - } - // For FabricUIManager only @Override @UiThread @@ -196,22 +172,6 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec return; } - // The problem we're trying to solve here: we could be in the middle of queueing - // a batch of related animation operations when Fabric flushes a batch of MountItems. - // It's visually bad if we execute half of the animation ops and then wait another frame - // (or more) to execute the rest. - // See mFrameNumber. If the dispatchedFrameNumber drifts too far - that - // is, if no MountItems are scheduled for a while, which can happen if a tree - // is committed but there are no changes - bring these counts back in sync and - // execute any queued operations. This number is arbitrary, but we want it low - // enough that the user shouldn't be able to see this delay in most cases. - mDispatchedFrameNumber++; - long currentFrameNo = mFrameNumber - 1; - if ((mDispatchedFrameNumber - mFrameNumber) > 2) { - mFrameNumber = mDispatchedFrameNumber; - currentFrameNo = mFrameNumber; - } - // This will execute all operations and preOperations queued // since the last time this was run, and will race with anything // being queued from the JS thread. That is, if the JS thread @@ -222,35 +182,13 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec // is that `scheduleMountItems` happens as close to the JS commit as // possible, whereas execution of those same items might happen sometime // later on the UI thread while the JS thread keeps plugging along. - executeAllOperations(mPreOperations, currentFrameNo); - executeAllOperations(mOperations, currentFrameNo); + executeAllOperations(mPreOperations); + executeAllOperations(mOperations); } - private void executeAllOperations(Queue operationQueue, long maxFrameNumber) { + private void executeAllOperations(Queue operationQueue) { NativeAnimatedNodesManager nodesManager = getNodesManager(); while (true) { - // There is a race condition where `peek` may return a non-null value and isEmpty() is false, - // but `poll` returns a null value - it's not clear why since we only peek and poll on the UI - // thread, but it might be something that happens during teardown or a crash. Regardless, the - // root cause is not currently known so we're extra cautious here. - // It happens equally in Fabric and non-Fabric. - UIThreadOperation peekedOperation = operationQueue.peek(); - - // This is the same as operationQueue.isEmpty() - if (peekedOperation == null) { - return; - } - // The rest of the operations are for the next frame. - if (peekedOperation.getFrameNumber() > maxFrameNumber) { - return; - } - - // Since we apparently can't guarantee that there is still an operation on the queue, - // much less the same operation, we do a poll and another null check. If this isn't - // the same operation as the peeked operation, we can't do anything about it - we still - // need to execute it, we have no mechanism to put it at the front of the queue, and it - // won't cause any errors to execute it earlier than expected (just a bit of UI jank at worst) - // so we just continue happily along. UIThreadOperation polledOperation = operationQueue.poll(); if (polledOperation == null) { return; @@ -270,13 +208,11 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec return; } - final long frameNo = mFrameNumber++; - UIBlock preOperationsUIBlock = new UIBlock() { @Override public void execute(NativeViewHierarchyManager nativeViewHierarchyManager) { - executeAllOperations(mPreOperations, frameNo); + executeAllOperations(mPreOperations); } }; @@ -284,7 +220,7 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec new UIBlock() { @Override public void execute(NativeViewHierarchyManager nativeViewHierarchyManager) { - executeAllOperations(mOperations, frameNo); + executeAllOperations(mOperations); } }; diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/UIManagerListener.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/UIManagerListener.java index c190e0a8c0..d08ab69b62 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/UIManagerListener.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/UIManagerListener.java @@ -16,6 +16,4 @@ public interface UIManagerListener { void willDispatchViewUpdates(UIManager uiManager); /* Called right after view updates are dispatched for a frame. */ void didDispatchMountItems(UIManager uiManager); - /* Called right after scheduleMountItems is called in Fabric, after a new tree is committed. */ - void didScheduleMountItems(UIManager uiManager); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java index 8538ce94c7..2cbaa95bf3 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -587,12 +587,6 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { boolean isBatchMountItem = mountItem instanceof BatchMountItem; boolean shouldSchedule = !(isBatchMountItem && ((BatchMountItem) mountItem).getSize() == 0); - // In case of sync rendering, this could be called on the UI thread. Otherwise, - // it should ~always be called on the JS thread. - for (UIManagerListener listener : mListeners) { - listener.didScheduleMountItems(this); - } - if (isBatchMountItem) { mCommitStartTime = commitStartTime; mLayoutTime = layoutEndTime - layoutStartTime; From 3c6c5f057a78bed7e69b0834ca0894f3b5261149 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Wed, 29 Jul 2020 17:40:05 -0700 Subject: [PATCH 6/7] Trivial: fix typos in log messages of NativeAnimatedModule Summary: Fix typos. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D22808108 fbshipit-source-id: 7a0406d902ac92bc27ecd49fd061704539266bf2 --- .../facebook/react/animated/NativeAnimatedModule.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java index dad43b29d7..e759b0269f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java @@ -670,10 +670,7 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec if (ANIMATED_MODULE_DEBUG) { FLog.d( NAME, - "queue connectAnimatedNodeToView: disconnectAnimatedNodeFromView: " - + animatedNodeTag - + " viewTag: " - + viewTag); + "queue: disconnectAnimatedNodeFromView: " + animatedNodeTag + " viewTag: " + viewTag); } decrementInFlightAnimationsForViewTag(viewTag); @@ -685,7 +682,7 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec if (ANIMATED_MODULE_DEBUG) { FLog.d( NAME, - "execute connectAnimatedNodeToView: disconnectAnimatedNodeFromView: " + "execute: disconnectAnimatedNodeFromView: " + animatedNodeTag + " viewTag: " + viewTag); @@ -762,7 +759,7 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec if (ANIMATED_MODULE_DEBUG) { FLog.d( NAME, - "queue addAnimatedEventToView: removeAnimatedEventFromView: " + "queue removeAnimatedEventFromView: viewTag: " + viewTag + " eventName: " + eventName @@ -779,7 +776,7 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec if (ANIMATED_MODULE_DEBUG) { FLog.d( NAME, - "execute addAnimatedEventToView: removeAnimatedEventFromView: " + "execute removeAnimatedEventFromView: viewTag: " + viewTag + " eventName: " + eventName From e1fa53af2eabc483be2f196ca96bbcd4a8226809 Mon Sep 17 00:00:00 2001 From: Will Holen Date: Wed, 29 Jul 2020 17:49:46 -0700 Subject: [PATCH 7/7] Implement Debugger.setBreakpointsActive Summary: Changelog: [Internal][Added] Support for toggling all breakpoints in Chrome debuggers This implements the Debugger.setBreakpointsActive CDP message, allowing users in Chrome to toggle all exceptions on and off. As with V8/Chrome, setting a breakpoint will automatically re-activate all breakpoints. #utd-hermes-ignore-android Reviewed By: avp Differential Revision: D22825209 fbshipit-source-id: bda2cc59aba04443280bca46126c19bb0cdb58d7 --- ReactCommon/hermes/inspector/Inspector.cpp | 11 ++++ ReactCommon/hermes/inspector/Inspector.h | 9 +++ .../hermes/inspector/InspectorState.cpp | 6 ++ .../hermes/inspector/chrome/Connection.cpp | 11 ++++ .../hermes/inspector/chrome/MessageTypes.cpp | 33 +++++++++- .../hermes/inspector/chrome/MessageTypes.h | 15 ++++- .../chrome/tests/ConnectionTests.cpp | 64 +++++++++++++++++++ .../hermes/inspector/tools/message_types.txt | 1 + 8 files changed, 148 insertions(+), 2 deletions(-) diff --git a/ReactCommon/hermes/inspector/Inspector.cpp b/ReactCommon/hermes/inspector/Inspector.cpp index 14cb76cad3..21b7bd2108 100644 --- a/ReactCommon/hermes/inspector/Inspector.cpp +++ b/ReactCommon/hermes/inspector/Inspector.cpp @@ -351,6 +351,9 @@ folly::Future Inspector::setBreakpoint( debugger::SourceLocation loc, folly::Optional condition) { auto promise = std::make_shared>(); + // Automatically re-enable breakpoints since the user presumably wants this + // to start triggering. + breakpointsActive_ = true; executor_->add([this, loc, condition, promise] { setBreakpointOnExecutor(loc, condition, promise); @@ -453,6 +456,14 @@ folly::Future Inspector::setPauseOnLoads( return promise->getFuture(); }; +folly::Future Inspector::setBreakpointsActive(bool active) { + // Same logic as setPauseOnLoads. + auto promise = std::make_shared>(); + breakpointsActive_ = active; + promise->setValue(); + return promise->getFuture(); +}; + bool Inspector::shouldPauseOnThisScriptLoad() { switch (pauseOnLoadMode_) { case None: diff --git a/ReactCommon/hermes/inspector/Inspector.h b/ReactCommon/hermes/inspector/Inspector.h index ac00e670e7..5d98588286 100644 --- a/ReactCommon/hermes/inspector/Inspector.h +++ b/ReactCommon/hermes/inspector/Inspector.h @@ -208,6 +208,12 @@ class Inspector : public facebook::hermes::debugger::EventObserver, */ folly::Future setPauseOnLoads(const PauseOnLoadMode mode); + /** + * Set whether breakpoints are active (pause when hit). This does not require + * runtime modifications, but returns a future for consistency. + */ + folly::Future setBreakpointsActive(bool active); + /** * If called during a script load event, return true if we should pause. * Assumed to be called from a script load event where we already hold @@ -326,6 +332,9 @@ class Inspector : public facebook::hermes::debugger::EventObserver, // Whether we should enter a paused state when a script loads. PauseOnLoadMode pauseOnLoadMode_ = PauseOnLoadMode::None; + // Whether or not we should pause on breakpoints. + bool breakpointsActive_ = true; + // All scripts loaded in to the VM, along with whether we've notified the // client about the script yet. struct LoadedScriptInfo { diff --git a/ReactCommon/hermes/inspector/InspectorState.cpp b/ReactCommon/hermes/inspector/InspectorState.cpp index 3804287247..07e005ede5 100644 --- a/ReactCommon/hermes/inspector/InspectorState.cpp +++ b/ReactCommon/hermes/inspector/InspectorState.cpp @@ -254,6 +254,12 @@ std::pair InspectorState::Running::didPause( pendingEvalPromise_->setValue( inspector_.debugger_.getProgramState().getEvalResult()); pendingEvalPromise_.reset(); + } else if ( + reason == debugger::PauseReason::Breakpoint && + !inspector_.breakpointsActive_) { + // We hit a user defined breakpoint, but breakpoints have been deactivated. + return std::make_pair( + nullptr, makeContinueCommand()); } else /* other cases imply a transition to Pause */ { return std::make_pair( InspectorState::Paused::make(inspector_), nullptr); diff --git a/ReactCommon/hermes/inspector/chrome/Connection.cpp b/ReactCommon/hermes/inspector/chrome/Connection.cpp index 43dacde5bf..e8653e592c 100644 --- a/ReactCommon/hermes/inspector/chrome/Connection.cpp +++ b/ReactCommon/hermes/inspector/chrome/Connection.cpp @@ -81,6 +81,7 @@ class Connection::Impl : public inspector::InspectorObserver, void handle(const m::debugger::ResumeRequest &req) override; void handle(const m::debugger::SetBreakpointRequest &req) override; void handle(const m::debugger::SetBreakpointByUrlRequest &req) override; + void handle(const m::debugger::SetBreakpointsActiveRequest &req) override; void handle( const m::debugger::SetInstrumentationBreakpointRequest &req) override; void handle(const m::debugger::SetPauseOnExceptionsRequest &req) override; @@ -659,6 +660,16 @@ void Connection::Impl::handle( .thenError(sendErrorToClient(req.id)); } +void Connection::Impl::handle( + const m::debugger::SetBreakpointsActiveRequest &req) { + inspector_->setBreakpointsActive(req.active) + .via(executor_.get()) + .thenValue([this, id = req.id](const Unit &unit) { + sendResponseToClient(m::makeOkResponse(id)); + }) + .thenError(sendErrorToClient(req.id)); +} + bool Connection::Impl::isVirtualBreakpointId(const std::string &id) { return id.rfind(kVirtualBreakpointPrefix, 0) == 0; } diff --git a/ReactCommon/hermes/inspector/chrome/MessageTypes.cpp b/ReactCommon/hermes/inspector/chrome/MessageTypes.cpp index e136e1c8f1..fa97e98ed8 100644 --- a/ReactCommon/hermes/inspector/chrome/MessageTypes.cpp +++ b/ReactCommon/hermes/inspector/chrome/MessageTypes.cpp @@ -1,5 +1,5 @@ // Copyright 2004-present Facebook. All Rights Reserved. -// @generated SignedSource<> +// @generated SignedSource<> #include "MessageTypes.h" @@ -35,6 +35,8 @@ std::unique_ptr Request::fromJsonThrowOnError(const std::string &str) { {"Debugger.setBreakpoint", makeUnique}, {"Debugger.setBreakpointByUrl", makeUnique}, + {"Debugger.setBreakpointsActive", + makeUnique}, {"Debugger.setInstrumentationBreakpoint", makeUnique}, {"Debugger.setPauseOnExceptions", @@ -509,6 +511,35 @@ void debugger::SetBreakpointByUrlRequest::accept( handler.handle(*this); } +debugger::SetBreakpointsActiveRequest::SetBreakpointsActiveRequest() + : Request("Debugger.setBreakpointsActive") {} + +debugger::SetBreakpointsActiveRequest::SetBreakpointsActiveRequest( + const dynamic &obj) + : Request("Debugger.setBreakpointsActive") { + assign(id, obj, "id"); + assign(method, obj, "method"); + + dynamic params = obj.at("params"); + assign(active, params, "active"); +} + +dynamic debugger::SetBreakpointsActiveRequest::toDynamic() const { + dynamic params = dynamic::object; + put(params, "active", active); + + dynamic obj = dynamic::object; + put(obj, "id", id); + put(obj, "method", method); + put(obj, "params", std::move(params)); + return obj; +} + +void debugger::SetBreakpointsActiveRequest::accept( + RequestHandler &handler) const { + handler.handle(*this); +} + debugger::SetInstrumentationBreakpointRequest:: SetInstrumentationBreakpointRequest() : Request("Debugger.setInstrumentationBreakpoint") {} diff --git a/ReactCommon/hermes/inspector/chrome/MessageTypes.h b/ReactCommon/hermes/inspector/chrome/MessageTypes.h index d7e38d81a4..fdb6270f11 100644 --- a/ReactCommon/hermes/inspector/chrome/MessageTypes.h +++ b/ReactCommon/hermes/inspector/chrome/MessageTypes.h @@ -1,5 +1,5 @@ // Copyright 2004-present Facebook. All Rights Reserved. -// @generated SignedSource<<356df52df2a053b5254f0e039cc36a7b>> +// @generated SignedSource<<0563169b47d73a70d7540528f28d1d13>> #pragma once @@ -38,6 +38,7 @@ struct SetBreakpointByUrlRequest; struct SetBreakpointByUrlResponse; struct SetBreakpointRequest; struct SetBreakpointResponse; +struct SetBreakpointsActiveRequest; struct SetInstrumentationBreakpointRequest; struct SetInstrumentationBreakpointResponse; struct SetPauseOnExceptionsRequest; @@ -89,6 +90,7 @@ struct RequestHandler { virtual void handle(const debugger::ResumeRequest &req) = 0; virtual void handle(const debugger::SetBreakpointRequest &req) = 0; virtual void handle(const debugger::SetBreakpointByUrlRequest &req) = 0; + virtual void handle(const debugger::SetBreakpointsActiveRequest &req) = 0; virtual void handle( const debugger::SetInstrumentationBreakpointRequest &req) = 0; virtual void handle(const debugger::SetPauseOnExceptionsRequest &req) = 0; @@ -116,6 +118,7 @@ struct NoopRequestHandler : public RequestHandler { void handle(const debugger::ResumeRequest &req) override {} void handle(const debugger::SetBreakpointRequest &req) override {} void handle(const debugger::SetBreakpointByUrlRequest &req) override {} + void handle(const debugger::SetBreakpointsActiveRequest &req) override {} void handle( const debugger::SetInstrumentationBreakpointRequest &req) override {} void handle(const debugger::SetPauseOnExceptionsRequest &req) override {} @@ -354,6 +357,16 @@ struct debugger::SetBreakpointByUrlRequest : public Request { folly::Optional condition; }; +struct debugger::SetBreakpointsActiveRequest : public Request { + SetBreakpointsActiveRequest(); + explicit SetBreakpointsActiveRequest(const folly::dynamic &obj); + + folly::dynamic toDynamic() const override; + void accept(RequestHandler &handler) const override; + + bool active{}; +}; + struct debugger::SetInstrumentationBreakpointRequest : public Request { SetInstrumentationBreakpointRequest(); explicit SetInstrumentationBreakpointRequest(const folly::dynamic &obj); diff --git a/ReactCommon/hermes/inspector/chrome/tests/ConnectionTests.cpp b/ReactCommon/hermes/inspector/chrome/tests/ConnectionTests.cpp index 9d5abc5962..b97bf2cccd 100644 --- a/ReactCommon/hermes/inspector/chrome/tests/ConnectionTests.cpp +++ b/ReactCommon/hermes/inspector/chrome/tests/ConnectionTests.cpp @@ -750,6 +750,70 @@ TEST(ConnectionTests, testSetBreakpointById) { expectNotification(conn); } +TEST(ConnectionTests, testActivateBreakpoints) { + TestContext context; + AsyncHermesRuntime &asyncRuntime = context.runtime(); + SyncConnection &conn = context.conn(); + int msgId = 1; + + asyncRuntime.executeScriptAsync(R"( + debugger; // line 1 + x=100 // 2 + debugger; // 3 + x=101; // 4 + )"); + + send(conn, ++msgId); + expectExecutionContextCreated(conn); + auto script = expectNotification(conn); + + expectPaused(conn, "other", {{"global", 1, 1}}); + + // Set breakpoint #1 + m::debugger::SetBreakpointRequest req; + req.id = ++msgId; + req.location.scriptId = script.scriptId; + req.location.lineNumber = 2; + conn.send(req.toJson()); + expectResponse(conn, req.id); + + // Set breakpoint #2 + req.id = ++msgId; + req.location.scriptId = script.scriptId; + req.location.lineNumber = 4; + conn.send(req.toJson()); + expectResponse(conn, req.id); + + // Disable breakpoints + m::debugger::SetBreakpointsActiveRequest activeReq; + activeReq.id = ++msgId; + activeReq.active = false; + conn.send(activeReq.toJson()); + expectResponse(conn, activeReq.id); + + // Resume + send(conn, ++msgId); + expectNotification(conn); + + // Expect first breakpoint to be skipped, now hitting line #3 + expectPaused(conn, "other", {{"global", 3, 1}}); + + // Re-enable breakpoints + activeReq.id = ++msgId; + activeReq.active = true; + conn.send(activeReq.toJson()); + expectResponse(conn, activeReq.id); + + // Resume and expect breakpoints to trigger again + send(conn, ++msgId); + expectNotification(conn); + expectPaused(conn, "other", {{"global", 4, 1}}); + + // Continue and exit + send(conn, ++msgId); + expectNotification(conn); +} + TEST(ConnectionTests, testSetBreakpointByIdWithColumnInIndenting) { TestContext context; AsyncHermesRuntime &asyncRuntime = context.runtime(); diff --git a/ReactCommon/hermes/inspector/tools/message_types.txt b/ReactCommon/hermes/inspector/tools/message_types.txt index 02efcce5f1..7807ac5b01 100644 --- a/ReactCommon/hermes/inspector/tools/message_types.txt +++ b/ReactCommon/hermes/inspector/tools/message_types.txt @@ -10,6 +10,7 @@ Debugger.resumed Debugger.scriptParsed Debugger.setBreakpoint Debugger.setBreakpointByUrl +Debugger.setBreakpointsActive Debugger.setInstrumentationBreakpoint Debugger.setPauseOnExceptions Debugger.stepInto