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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unify feature flags between fb and oss for React Native renderer #28269

Conversation

sammy-SC
Copy link
Contributor

@sammy-SC sammy-SC commented Feb 7, 2024

Affected flags:

alwaysThrottleRetries

In RN OSS changed from true to false. This is what FB build uses. This flag was a root cause for big perf regression internally.

enableDeferRootSchedulingToMicrotask

In RN OSS build changed from true to false. This is what FB build uses.

debugRenderPhaseSideEffectsForStrictMode

Changed from true to DEV in FB and OSS build. The flag is only used in debug builds and was previously false in RN OSS builds

enableUnifiedSyncLane

Changed from __VARIANT__ to true in FB build. This is what OSS build uses. This flag is shipped internally. cc @tyao1

enableLegacyHidden

In RN FB changed from true to false. This is what OSS uses.

allowConcurrentByDefault

In RN FB changed from true to false.

I ran yarn flags --diff rn rn-fb to get the difference between feature flags and unify them.

Before

yarn run v1.22.19
$ node ./scripts/flags/flags.js --diff rn rn-fb
┌───────────────────────────────────────────────┬────────┬───────┐
│                    (index)                    │ RN OSS │ RN FB │
├───────────────────────────────────────────────┼────────┼───────┤
│ allowConcurrentByDefault                      │  '❌'  │ '✅'  │
│ debugRenderPhaseSideEffectsForStrictMode      │  '❌'  │ '✅'  │
│ disableModulePatternComponents                │  '❌'  │ '✅'  │
│ enableCPUSuspense                             │  '❌'  │ '✅'  │
│ enableCacheElement                            │  '❌'  │ '✅'  │
│ enableGetInspectorDataForInstanceInProduction │  '❌'  │ '✅'  │
│ enableLegacyHidden                            │  '❌'  │ '✅'  │
│ enableSchedulingProfiler                      │  '❌'  │ '📊'  │
│ enableUseDeferredValueInitialArg              │  '❌'  │ '✅'  │
│ enableUseMemoCacheHook                        │  '❌'  │ '✅'  │
│ enableUseRefAccessWarning                     │  '❌'  │ '🧪'  │
│ passChildrenWhenCloningPersistedNodes         │  '❌'  │ '🧪'  │
│ useMicrotasksForSchedulingInFabric            │  '❌'  │ '🧪'  │
│ alwaysThrottleRetries                         │  '✅'  │ '🧪'  │
│ enableDeferRootSchedulingToMicrotask          │  '✅'  │ '🧪'  │
│ enableUnifiedSyncLane                         │  '✅'  │ '🧪'  │
└───────────────────────────────────────────────┴────────┴───────┘

After

yarn run v1.22.19
$ node ./scripts/flags/flags.js --diff rn rn-fb
┌───────────────────────────────────────────────┬────────┬───────┐
│                    (index)                    │ RN OSS │ RN FB │
├───────────────────────────────────────────────┼────────┼───────┤
│ alwaysThrottleRetries                         │  '❌'  │ '🧪'  │
│ disableModulePatternComponents                │  '❌'  │ '✅'  │
│ enableCPUSuspense                             │  '❌'  │ '✅'  │
│ enableCacheElement                            │  '❌'  │ '✅'  │
│ enableDeferRootSchedulingToMicrotask          │  '❌'  │ '🧪'  │
│ enableGetInspectorDataForInstanceInProduction │  '❌'  │ '✅'  │
│ enableSchedulingProfiler                      │  '❌'  │ '📊'  │
│ enableUseDeferredValueInitialArg              │  '❌'  │ '✅'  │
│ enableUseMemoCacheHook                        │  '❌'  │ '✅'  │
│ enableUseRefAccessWarning                     │  '❌'  │ '🧪'  │
│ passChildrenWhenCloningPersistedNodes         │  '❌'  │ '🧪'  │
│ useMicrotasksForSchedulingInFabric            │  '❌'  │ '🧪'  │
└───────────────────────────────────────────────┴────────┴───────┘

@sammy-SC sammy-SC marked this pull request as draft February 7, 2024 12:30
@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Feb 7, 2024
@react-sizebot
Copy link

react-sizebot commented Feb 7, 2024

Comparing: bfdc12c...329f58d

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 176.66 kB 176.66 kB = 55.01 kB 55.01 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 178.65 kB 178.65 kB = 55.59 kB 55.59 kB
facebook-www/ReactDOM-prod.classic.js = 591.78 kB 591.78 kB = 104.44 kB 104.44 kB
facebook-www/ReactDOM-prod.modern.js = 575.53 kB 575.53 kB = 101.54 kB 101.54 kB
facebook-react-native/react-is/cjs/ReactIs-prod.js = 4.73 kB 4.63 kB = 1.18 kB 1.15 kB
facebook-react-native/react-is/cjs/ReactIs-profiling.js = 4.73 kB 4.63 kB = 1.18 kB 1.15 kB
test_utils/ReactAllWarnings.js Deleted 67.02 kB 0.00 kB Deleted 16.42 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
react-native/implementations/ReactFabric-dev.js +0.28% 981.18 kB 983.90 kB +0.17% 198.37 kB 198.70 kB
react-native/implementations/ReactNativeRenderer-dev.js +0.27% 997.46 kB 1,000.18 kB +0.17% 202.66 kB 203.00 kB
react-native/implementations/ReactNativeRenderer-dev.fb.js = 1,022.86 kB 1,020.29 kB = 206.67 kB 206.25 kB
react-native/implementations/ReactFabric-dev.fb.js = 1,008.26 kB 1,005.68 kB = 202.84 kB 202.39 kB
react-native/implementations/ReactNativeRenderer-profiling.fb.js = 364.42 kB 362.67 kB = 64.04 kB 63.72 kB
react-native/implementations/ReactFabric-profiling.fb.js = 356.89 kB 355.14 kB = 62.62 kB 62.32 kB
react-native/implementations/ReactNativeRenderer-prod.fb.js = 337.17 kB 335.42 kB = 59.80 kB 59.47 kB
react-native/implementations/ReactFabric-prod.fb.js = 329.65 kB 327.90 kB = 58.43 kB 58.11 kB
facebook-react-native/react-is/cjs/ReactIs-dev.js = 7.98 kB 7.93 kB = 1.98 kB 1.97 kB
facebook-react-native/react-is/cjs/ReactIs-prod.js = 4.73 kB 4.63 kB = 1.18 kB 1.15 kB
facebook-react-native/react-is/cjs/ReactIs-profiling.js = 4.73 kB 4.63 kB = 1.18 kB 1.15 kB
test_utils/ReactAllWarnings.js Deleted 67.02 kB 0.00 kB Deleted 16.42 kB 0.00 kB

Generated by 🚫 dangerJS against 329f58d

@sammy-SC sammy-SC force-pushed the sammy/unify-feature-flags-between-fb-and-oss-for-rn branch 3 times, most recently from d1a0f59 to 5813b3d Compare February 7, 2024 13:50
@sammy-SC sammy-SC marked this pull request as ready for review February 7, 2024 14:12
Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

The right fix for these depends on the flag.

These are good:

  • enableUnifiedSyncLane
  • debugRenderPhaseSideEffectsForStrictMode

These need to land at FB

The right fix for these to to land them at FB, not change them in OSS.

  • allowConcurrentByDefault: RN OSS shouldn't use concurrent by default. This is only on in RN FB because landing it breaks relay tests. We should fix the relay tests and turn this off at FB.
  • alwaysThrottleRetries - do we have a plan to land this? What was the regression and was it FB specific? RN OSS isn't using suspense so I think we should leave this on.
  • enableDeferRootSchedulingToMicrotask - we should land this at FB and keep the OSS one on.

These are experimental, should stay enabled in Fb build

These are experimental and should stay on in the FB builds so we can catch any unintentional regressions. If the changes are limited to only when the API is used, then keeping this on internally is harmless. But if there are, and we miss them, turning it on later could be difficult

  • enableCPUSuspense
  • enableCacheElement

Can't turn on 19 features

These are 19 features that will break paper, so we need to decide what to do here. Turning it off at FB doesn't seem like the right move, since FB is on 19 already. We should roll the feature out to RN OSS in 19:

  • disableModulePatternComponents
  • enableUseDeferredValueInitialArg

Not sure about these

  • enableLegacyHidden: what does this do? Is it a breaking change to turn it off?
  • enableUseMemoCacheHook: we have not decided whether we're including this in the 19 release in OSS yet. It's only experimental in React, so we shouldn't ship it to RN OSS yet.

@sammy-SC sammy-SC force-pushed the sammy/unify-feature-flags-between-fb-and-oss-for-rn branch 2 times, most recently from 4e81bba to 8c6b970 Compare February 8, 2024 15:12
Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for doing this!

@sammy-SC sammy-SC force-pushed the sammy/unify-feature-flags-between-fb-and-oss-for-rn branch from 8c6b970 to c00f61f Compare February 8, 2024 18:26
@sammy-SC sammy-SC force-pushed the sammy/unify-feature-flags-between-fb-and-oss-for-rn branch from c00f61f to 329f58d Compare February 8, 2024 18:31
@sammy-SC sammy-SC merged commit 36b078c into facebook:main Feb 9, 2024
36 checks passed
@sammy-SC sammy-SC deleted the sammy/unify-feature-flags-between-fb-and-oss-for-rn branch February 9, 2024 09:46
huozhi added a commit to vercel/next.js that referenced this pull request Feb 23, 2024
### React upstream changes

- facebook/react#28333
- facebook/react#28334
- facebook/react#28378
- facebook/react#28377
- facebook/react#28376
- facebook/react#28338
- facebook/react#28331
- facebook/react#28336
- facebook/react#28320
- facebook/react#28317
- facebook/react#28375
- facebook/react#28367
- facebook/react#28380
- facebook/react#28368
- facebook/react#28343
- facebook/react#28355
- facebook/react#28374
- facebook/react#28362
- facebook/react#28344
- facebook/react#28339
- facebook/react#28353
- facebook/react#28346
- facebook/react#25790
- facebook/react#28352
- facebook/react#28326
- facebook/react#27688
- facebook/react#28329
- facebook/react#28332
- facebook/react#28340
- facebook/react#28327
- facebook/react#28325
- facebook/react#28324
- facebook/react#28309
- facebook/react#28310
- facebook/react#28307
- facebook/react#28306
- facebook/react#28315
- facebook/react#28318
- facebook/react#28226
- facebook/react#28308
- facebook/react#27563
- facebook/react#28297
- facebook/react#28286
- facebook/react#28284
- facebook/react#28275
- facebook/react#28145
- facebook/react#28301
- facebook/react#28224
- facebook/react#28152
- facebook/react#28296
- facebook/react#28294
- facebook/react#28279
- facebook/react#28273
- facebook/react#28269
- facebook/react#28376
- facebook/react#28338
- facebook/react#28331
- facebook/react#28336
- facebook/react#28320
- facebook/react#28317
- facebook/react#28375
- facebook/react#28367
- facebook/react#28380
- facebook/react#28368
- facebook/react#28343
- facebook/react#28355
- facebook/react#28374
- facebook/react#28362
- facebook/react#28344
- facebook/react#28339
- facebook/react#28353
- facebook/react#28346
- facebook/react#25790
- facebook/react#28352
- facebook/react#28326
- facebook/react#27688
- facebook/react#28329
- facebook/react#28332
- facebook/react#28340
- facebook/react#28327
- facebook/react#28325
- facebook/react#28324
- facebook/react#28309
- facebook/react#28310
- facebook/react#28307
- facebook/react#28306
- facebook/react#28315
- facebook/react#28318
- facebook/react#28226
- facebook/react#28308
- facebook/react#27563
- facebook/react#28297
- facebook/react#28286
- facebook/react#28284
- facebook/react#28275
- facebook/react#28145
- facebook/react#28301
- facebook/react#28224
- facebook/react#28152
- facebook/react#28296
- facebook/react#28294
- facebook/react#28279
- facebook/react#28273
- facebook/react#28269

Closes NEXT-2542


Disable ppr test for strict mode for now, @acdlite will check it and
we'll sync again
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…ebook#28269)

# Affected flags:

### alwaysThrottleRetries

In RN OSS changed from `true` to `false`. This is what FB build uses.
This flag was a root cause for big perf regression internally.

### enableDeferRootSchedulingToMicrotask

In RN OSS build changed from `true` to `false`. This is what FB build
uses.

### debugRenderPhaseSideEffectsForStrictMode

Changed from true to __DEV__ in FB and OSS build. The flag is only used
in debug builds and was previously `false` in RN OSS builds

### enableUnifiedSyncLane

Changed from `__VARIANT__` to `true` in FB build. This is what OSS build
uses. This flag is shipped internally. cc @tyao1

### enableLegacyHidden

In RN FB changed from `true` to `false`.  This is what OSS uses.

### allowConcurrentByDefault

In RN FB changed from `true` to `false`. 




I ran `yarn flags --diff rn rn-fb` to get the difference between feature
flags and unify them.

## Before
```
yarn run v1.22.19
$ node ./scripts/flags/flags.js --diff rn rn-fb
┌───────────────────────────────────────────────┬────────┬───────┐
│                    (index)                    │ RN OSS │ RN FB │
├───────────────────────────────────────────────┼────────┼───────┤
│ allowConcurrentByDefault                      │  '❌'  │ '✅'  │
│ debugRenderPhaseSideEffectsForStrictMode      │  '❌'  │ '✅'  │
│ disableModulePatternComponents                │  '❌'  │ '✅'  │
│ enableCPUSuspense                             │  '❌'  │ '✅'  │
│ enableCacheElement                            │  '❌'  │ '✅'  │
│ enableGetInspectorDataForInstanceInProduction │  '❌'  │ '✅'  │
│ enableLegacyHidden                            │  '❌'  │ '✅'  │
│ enableSchedulingProfiler                      │  '❌'  │ '📊'  │
│ enableUseDeferredValueInitialArg              │  '❌'  │ '✅'  │
│ enableUseMemoCacheHook                        │  '❌'  │ '✅'  │
│ enableUseRefAccessWarning                     │  '❌'  │ '🧪'  │
│ passChildrenWhenCloningPersistedNodes         │  '❌'  │ '🧪'  │
│ useMicrotasksForSchedulingInFabric            │  '❌'  │ '🧪'  │
│ alwaysThrottleRetries                         │  '✅'  │ '🧪'  │
│ enableDeferRootSchedulingToMicrotask          │  '✅'  │ '🧪'  │
│ enableUnifiedSyncLane                         │  '✅'  │ '🧪'  │
└───────────────────────────────────────────────┴────────┴───────┘
```

## After
```
yarn run v1.22.19
$ node ./scripts/flags/flags.js --diff rn rn-fb
┌───────────────────────────────────────────────┬────────┬───────┐
│                    (index)                    │ RN OSS │ RN FB │
├───────────────────────────────────────────────┼────────┼───────┤
│ alwaysThrottleRetries                         │  '❌'  │ '🧪'  │
│ disableModulePatternComponents                │  '❌'  │ '✅'  │
│ enableCPUSuspense                             │  '❌'  │ '✅'  │
│ enableCacheElement                            │  '❌'  │ '✅'  │
│ enableDeferRootSchedulingToMicrotask          │  '❌'  │ '🧪'  │
│ enableGetInspectorDataForInstanceInProduction │  '❌'  │ '✅'  │
│ enableSchedulingProfiler                      │  '❌'  │ '📊'  │
│ enableUseDeferredValueInitialArg              │  '❌'  │ '✅'  │
│ enableUseMemoCacheHook                        │  '❌'  │ '✅'  │
│ enableUseRefAccessWarning                     │  '❌'  │ '🧪'  │
│ passChildrenWhenCloningPersistedNodes         │  '❌'  │ '🧪'  │
│ useMicrotasksForSchedulingInFabric            │  '❌'  │ '🧪'  │
└───────────────────────────────────────────────┴────────┴───────┘
```
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
)

# Affected flags:

### alwaysThrottleRetries

In RN OSS changed from `true` to `false`. This is what FB build uses.
This flag was a root cause for big perf regression internally.

### enableDeferRootSchedulingToMicrotask

In RN OSS build changed from `true` to `false`. This is what FB build
uses.

### debugRenderPhaseSideEffectsForStrictMode

Changed from true to __DEV__ in FB and OSS build. The flag is only used
in debug builds and was previously `false` in RN OSS builds

### enableUnifiedSyncLane

Changed from `__VARIANT__` to `true` in FB build. This is what OSS build
uses. This flag is shipped internally. cc @tyao1

### enableLegacyHidden

In RN FB changed from `true` to `false`.  This is what OSS uses.

### allowConcurrentByDefault

In RN FB changed from `true` to `false`.

I ran `yarn flags --diff rn rn-fb` to get the difference between feature
flags and unify them.

## Before
```
yarn run v1.22.19
$ node ./scripts/flags/flags.js --diff rn rn-fb
┌───────────────────────────────────────────────┬────────┬───────┐
│                    (index)                    │ RN OSS │ RN FB │
├───────────────────────────────────────────────┼────────┼───────┤
│ allowConcurrentByDefault                      │  '❌'  │ '✅'  │
│ debugRenderPhaseSideEffectsForStrictMode      │  '❌'  │ '✅'  │
│ disableModulePatternComponents                │  '❌'  │ '✅'  │
│ enableCPUSuspense                             │  '❌'  │ '✅'  │
│ enableCacheElement                            │  '❌'  │ '✅'  │
│ enableGetInspectorDataForInstanceInProduction │  '❌'  │ '✅'  │
│ enableLegacyHidden                            │  '❌'  │ '✅'  │
│ enableSchedulingProfiler                      │  '❌'  │ '📊'  │
│ enableUseDeferredValueInitialArg              │  '❌'  │ '✅'  │
│ enableUseMemoCacheHook                        │  '❌'  │ '✅'  │
│ enableUseRefAccessWarning                     │  '❌'  │ '🧪'  │
│ passChildrenWhenCloningPersistedNodes         │  '❌'  │ '🧪'  │
│ useMicrotasksForSchedulingInFabric            │  '❌'  │ '🧪'  │
│ alwaysThrottleRetries                         │  '✅'  │ '🧪'  │
│ enableDeferRootSchedulingToMicrotask          │  '✅'  │ '🧪'  │
│ enableUnifiedSyncLane                         │  '✅'  │ '🧪'  │
└───────────────────────────────────────────────┴────────┴───────┘
```

## After
```
yarn run v1.22.19
$ node ./scripts/flags/flags.js --diff rn rn-fb
┌───────────────────────────────────────────────┬────────┬───────┐
│                    (index)                    │ RN OSS │ RN FB │
├───────────────────────────────────────────────┼────────┼───────┤
│ alwaysThrottleRetries                         │  '❌'  │ '🧪'  │
│ disableModulePatternComponents                │  '❌'  │ '✅'  │
│ enableCPUSuspense                             │  '❌'  │ '✅'  │
│ enableCacheElement                            │  '❌'  │ '✅'  │
│ enableDeferRootSchedulingToMicrotask          │  '❌'  │ '🧪'  │
│ enableGetInspectorDataForInstanceInProduction │  '❌'  │ '✅'  │
│ enableSchedulingProfiler                      │  '❌'  │ '📊'  │
│ enableUseDeferredValueInitialArg              │  '❌'  │ '✅'  │
│ enableUseMemoCacheHook                        │  '❌'  │ '✅'  │
│ enableUseRefAccessWarning                     │  '❌'  │ '🧪'  │
│ passChildrenWhenCloningPersistedNodes         │  '❌'  │ '🧪'  │
│ useMicrotasksForSchedulingInFabric            │  '❌'  │ '🧪'  │
└───────────────────────────────────────────────┴────────┴───────┘
```

DiffTrain build for commit 36b078c.
jackpope added a commit that referenced this pull request Jun 24, 2024
`enableUnifiedSyncLane` now passes everywhere. Let's clean it up

Implemented with #27646

Flag enabled with #27646,
#28269,
#29052
github-actions bot pushed a commit that referenced this pull request Jun 24, 2024
`enableUnifiedSyncLane` now passes everywhere. Let's clean it up

Implemented with #27646

Flag enabled with #27646,
#28269,
#29052

DiffTrain build for [c21bcd6](c21bcd6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants