Skip to content

Commit

Permalink
Mitigate a corner case of ReactNativeFeatureFlags late initialization…
Browse files Browse the repository at this point in the history
… order (#39106)

Summary:
Pull Request resolved: #39106

## Changelog:
[Internal] -

`ReactNativeFeatureFlags` are not guaranteed to be initialized before everything else, and one case popped up with this being a problem, in particular when creating `GlobalPerformanceLogger` instance (which may be created quite early on).

If the order of initialization of `ReactNativeFeatureFlags` and reading a value from there is flipped, then we'd just get a default value, without using any `MobileConfig` backing or anything.

This diff works this around for the particular case of `GlobalPerformanceLogger` initialization by making the corresponding flag tri-state (unititalized/true/false) and making sure that its value is only really taken into account after its initialized.

Reviewed By: ryancat

Differential Revision: D48559981

fbshipit-source-id: 7fa842d3b845866e15f89731c7e5c9036b83fb24
  • Loading branch information
rshest authored and facebook-github-bot committed Aug 22, 2023
1 parent 9edbd78 commit 6043960
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,10 @@ export type FeatureFlags = {|
animatedShouldUseSingleOp: () => boolean,
/**
* Enables GlobalPerformanceLogger replacement with a WebPerformance API based
* implementation
* implementation. Tri-state due to being sensitive to initialization order
* vs the platform-specific ReactNativeFeatureFlags implementation.
*/
isGlobalWebPerformanceLoggerEnabled: () => boolean,
isGlobalWebPerformanceLoggerEnabled: () => ?boolean,
/**
* Enables access to the host tree in Fabric using DOM-compatible APIs.
*/
Expand Down Expand Up @@ -69,7 +70,7 @@ const ReactNativeFeatureFlags: FeatureFlags = {
shouldPressibilityUseW3CPointerEventsForHover: () => false,
animatedShouldDebounceQueueFlush: () => false,
animatedShouldUseSingleOp: () => false,
isGlobalWebPerformanceLoggerEnabled: () => false,
isGlobalWebPerformanceLoggerEnabled: () => undefined,
enableAccessToHostTreeInFabric: () => false,
shouldUseAnimatedObjectForTransform: () => false,
shouldUseSetNativePropsInFabric: () => false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,26 +10,16 @@

import type {IPerformanceLogger} from './createPerformanceLogger';

import ReactNativeFeatureFlags from '../ReactNative/ReactNativeFeatureFlags';
import NativePerformance from '../WebPerformance/NativePerformance';
import createPerformanceLogger from './createPerformanceLogger';

function isLoggingForWebPerformance(): boolean {
return (
NativePerformance != null &&
ReactNativeFeatureFlags.isGlobalWebPerformanceLoggerEnabled()
);
}

/**
* This is a global shared instance of IPerformanceLogger that is created with
* createPerformanceLogger().
* This logger should be used only for global performance metrics like the ones
* that are logged during loading bundle. If you want to log something from your
* React component you should use PerformanceLoggerContext instead.
*/
const GlobalPerformanceLogger: IPerformanceLogger = createPerformanceLogger(
isLoggingForWebPerformance(),
);
const GlobalPerformanceLogger: IPerformanceLogger =
createPerformanceLogger(true);

module.exports = GlobalPerformanceLogger;
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import type {
} from './IPerformanceLogger';

import * as Systrace from '../Performance/Systrace';
import ReactNativeFeatureFlags from '../ReactNative/ReactNativeFeatureFlags';
import NativePerformance from '../WebPerformance/NativePerformance';
import infoLog from './infoLog';

const _cookies: {[key: string]: number, ...} = {};
Expand All @@ -35,10 +37,22 @@ class PerformanceLogger implements IPerformanceLogger {
_points: {[key: string]: ?number} = {};
_pointExtras: {[key: string]: ?Extras, ...} = {};
_closed: boolean = false;
_isLoggingForWebPerformance: boolean = false;
_isGlobalLogger: boolean = false;
_isGlobalWebPerformanceLoggerEnabled: ?boolean;

constructor(isLoggingForWebPerformance?: boolean) {
this._isLoggingForWebPerformance = isLoggingForWebPerformance === true;
constructor(isGlobalLogger?: boolean) {
this._isGlobalLogger = isGlobalLogger === true;
}

_isLoggingForWebPerformance(): boolean {
if (!this._isGlobalLogger || NativePerformance == null) {
return false;
}
if (this._isGlobalWebPerformanceLoggerEnabled == null) {
this._isGlobalWebPerformanceLoggerEnabled =
ReactNativeFeatureFlags.isGlobalWebPerformanceLoggerEnabled();
}
return this._isGlobalWebPerformanceLoggerEnabled === true;
}

// NOTE: The Performance.mark/measure calls are wrapped here to ensure that
Expand All @@ -49,7 +63,7 @@ class PerformanceLogger implements IPerformanceLogger {
// In most of the other cases this kind of check for `performance` being defined
// wouldn't be necessary.
_performanceMark(key: string, startTime: number) {
if (this._isLoggingForWebPerformance) {
if (this._isLoggingForWebPerformance()) {
global.performance?.mark?.(key, {
startTime,
});
Expand All @@ -61,7 +75,7 @@ class PerformanceLogger implements IPerformanceLogger {
start: number | string,
end: number | string,
) {
if (this._isLoggingForWebPerformance) {
if (this._isLoggingForWebPerformance()) {
global.performance?.measure?.(key, {
start,
end,
Expand Down Expand Up @@ -351,7 +365,7 @@ export type {Extras, ExtraValue, IPerformanceLogger, Timespan};
* The loggers need to have minimal overhead since they're used in production.
*/
export default function createPerformanceLogger(
isLoggingForWebPerformance?: boolean,
isGlobalLogger?: boolean,
): IPerformanceLogger {
return new PerformanceLogger(isLoggingForWebPerformance);
return new PerformanceLogger(isGlobalLogger);
}

0 comments on commit 6043960

Please sign in to comment.