Skip to content

Commit

Permalink
Fix: Move destroy field to shared instance object (#26561)
Browse files Browse the repository at this point in the history 
This reverts commit d41e5c2.

DiffTrain build for [1d3ffd9](1d3ffd9)
  • Loading branch information
@kassens
kassens committed Apr 20, 2023
1 parent 38b20a3 commit 1f49af9
Show file tree
Hide file tree
Showing 18 changed files with 684 additions and 450 deletions.
2 changes: 1 addition & 1 deletion compiled/facebook-www/REVISION
View file
Original file line number Diff line number Diff line change
@@ -1 +1 @@
2f18eb0ac63fdcaed3f1d4e3c375a559a552c09a
1d3ffd9d9addb4aa0988b92ee6917c07c3c261ae
2 changes: 1 addition & 1 deletion compiled/facebook-www/React-dev.modern.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ if (
}
"use strict";

var ReactVersion = "18.3.0-www-modern-b6a03dc1";
var ReactVersion = "18.3.0-www-modern-c1d269df";

// ATTENTION
// When adding new symbols to this file,
Expand Down
66 changes: 45 additions & 21 deletions compiled/facebook-www/ReactART-dev.classic.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ function _assertThisInitialized(self) {
return self;
}

var ReactVersion = "18.3.0-www-classic-ccedb28a";
var ReactVersion = "18.3.0-www-classic-cd98b4c3";

var LegacyRoot = 0;
var ConcurrentRoot = 1;
Expand Down Expand Up @@ -7343,7 +7343,21 @@ var didWarnAboutUseWrappedInTryCatch;
{
didWarnAboutMismatchedHooksForComponent = new Set();
didWarnAboutUseWrappedInTryCatch = new Set();
} // These are set right before calling the component.
} // The effect "instance" is a shared object that remains the same for the entire
// lifetime of an effect. In Rust terms, a RefCell. We use it to store the
// "destroy" function that is returned from an effect, because that is stateful.
// The field is `undefined` if the effect is unmounted, or if the effect ran
// but is not stateful. We don't explicitly track whether the effect is mounted
// or unmounted because that can be inferred by the hiddenness of the fiber in
// the tree, i.e. whether there is a hidden Offscreen fiber above it.
//
// It's unfortunate that this is stored on a separate object, because it adds
// more memory per effect instance, but it's conceptually sound. I think there's
// likely a better data structure we could use for effects; perhaps just one
// array of effect instances per fiber. But I think this is OK for now despite
// the additional memory and we can follow up with performance
// optimizations later.
// These are set right before calling the component.

var renderLanes$1 = NoLanes; // The work-in-progress fiber. I've named it differently to distinguish it from
// the work-in-progress hook.
Expand Down Expand Up @@ -8652,7 +8666,7 @@ function mountSyncExternalStore(subscribe, getSnapshot, getServerSnapshot) {
pushEffect(
HasEffect | Passive,
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
undefined,
createEffectInstance(),
null
);
return nextSnapshot;
Expand Down Expand Up @@ -8707,7 +8721,7 @@ function updateSyncExternalStore(subscribe, getSnapshot, getServerSnapshot) {
pushEffect(
HasEffect | Passive,
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
undefined,
createEffectInstance(),
null
); // Unless we're rendering a blocking lane, schedule a consistency check.
// Right before committing, we will walk the tree and check if any of the
Expand Down Expand Up @@ -8832,11 +8846,11 @@ function rerenderState(initialState) {
return rerenderReducer(basicStateReducer);
}

function pushEffect(tag, create, destroy, deps) {
function pushEffect(tag, create, inst, deps) {
var effect = {
tag: tag,
create: create,
destroy: destroy,
inst: inst,
deps: deps,
// Circular
next: null
Expand All @@ -8863,6 +8877,12 @@ function pushEffect(tag, create, destroy, deps) {
return effect;
}

function createEffectInstance() {
return {
destroy: undefined
};
}

var stackContainsErrorMessage = null;

function getCallerStackFrame() {
Expand Down Expand Up @@ -8963,25 +8983,24 @@ function mountEffectImpl(fiberFlags, hookFlags, create, deps) {
hook.memoizedState = pushEffect(
HasEffect | hookFlags,
create,
undefined,
createEffectInstance(),
nextDeps
);
}

function updateEffectImpl(fiberFlags, hookFlags, create, deps) {
var hook = updateWorkInProgressHook();
var nextDeps = deps === undefined ? null : deps;
var destroy = undefined; // currentHook is null when rerendering after a render phase state update.
var effect = hook.memoizedState;
var inst = effect.inst; // currentHook is null when rerendering after a render phase state update.

if (currentHook !== null) {
var prevEffect = currentHook.memoizedState;
destroy = prevEffect.destroy;

if (nextDeps !== null) {
var prevEffect = currentHook.memoizedState;
var prevDeps = prevEffect.deps;

if (areHookInputsEqual(nextDeps, prevDeps)) {
hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps);
hook.memoizedState = pushEffect(hookFlags, create, inst, nextDeps);
return;
}
}
Expand All @@ -8991,7 +9010,7 @@ function updateEffectImpl(fiberFlags, hookFlags, create, deps) {
hook.memoizedState = pushEffect(
HasEffect | hookFlags,
create,
destroy,
inst,
nextDeps
);
}
Expand Down Expand Up @@ -19298,10 +19317,12 @@ function commitHookEffectListUnmount(
do {
if ((effect.tag & flags) === flags) {
// Unmount
var destroy = effect.destroy;
effect.destroy = undefined;
var inst = effect.inst;
var destroy = inst.destroy;

if (destroy !== undefined) {
inst.destroy = undefined;

if (enableSchedulingProfiler) {
if ((flags & Passive) !== NoFlags) {
markComponentPassiveEffectUnmountStarted(finishedWork);
Expand Down Expand Up @@ -19365,7 +19386,9 @@ function commitHookEffectListMount(flags, finishedWork) {
}
}

effect.destroy = create();
var inst = effect.inst;
var destroy = create();
inst.destroy = destroy;

{
if ((flags & Insertion) !== NoFlags) {
Expand All @@ -19382,8 +19405,6 @@ function commitHookEffectListMount(flags, finishedWork) {
}

{
var destroy = effect.destroy;

if (destroy !== undefined && typeof destroy !== "function") {
var hookName = void 0;

Expand Down Expand Up @@ -20767,12 +20788,13 @@ function commitDeletionEffectsOnFiber(
var effect = firstEffect;

do {
var _effect = effect,
destroy = _effect.destroy,
tag = _effect.tag;
var tag = effect.tag;
var inst = effect.inst;
var destroy = inst.destroy;

if (destroy !== undefined) {
if ((tag & Insertion) !== NoFlags) {
inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
Expand All @@ -20785,13 +20807,15 @@ function commitDeletionEffectsOnFiber(

if (shouldProfile(deletedFiber)) {
startLayoutEffectTimer();
inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy
);
recordLayoutEffectDuration(deletedFiber);
} else {
inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
Expand Down
66 changes: 45 additions & 21 deletions compiled/facebook-www/ReactART-dev.modern.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ function _assertThisInitialized(self) {
return self;
}

var ReactVersion = "18.3.0-www-modern-89053274";
var ReactVersion = "18.3.0-www-modern-01966321";

var LegacyRoot = 0;
var ConcurrentRoot = 1;
Expand Down Expand Up @@ -7099,7 +7099,21 @@ var didWarnAboutUseWrappedInTryCatch;
{
didWarnAboutMismatchedHooksForComponent = new Set();
didWarnAboutUseWrappedInTryCatch = new Set();
} // These are set right before calling the component.
} // The effect "instance" is a shared object that remains the same for the entire
// lifetime of an effect. In Rust terms, a RefCell. We use it to store the
// "destroy" function that is returned from an effect, because that is stateful.
// The field is `undefined` if the effect is unmounted, or if the effect ran
// but is not stateful. We don't explicitly track whether the effect is mounted
// or unmounted because that can be inferred by the hiddenness of the fiber in
// the tree, i.e. whether there is a hidden Offscreen fiber above it.
//
// It's unfortunate that this is stored on a separate object, because it adds
// more memory per effect instance, but it's conceptually sound. I think there's
// likely a better data structure we could use for effects; perhaps just one
// array of effect instances per fiber. But I think this is OK for now despite
// the additional memory and we can follow up with performance
// optimizations later.
// These are set right before calling the component.

var renderLanes$1 = NoLanes; // The work-in-progress fiber. I've named it differently to distinguish it from
// the work-in-progress hook.
Expand Down Expand Up @@ -8408,7 +8422,7 @@ function mountSyncExternalStore(subscribe, getSnapshot, getServerSnapshot) {
pushEffect(
HasEffect | Passive,
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
undefined,
createEffectInstance(),
null
);
return nextSnapshot;
Expand Down Expand Up @@ -8463,7 +8477,7 @@ function updateSyncExternalStore(subscribe, getSnapshot, getServerSnapshot) {
pushEffect(
HasEffect | Passive,
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
undefined,
createEffectInstance(),
null
); // Unless we're rendering a blocking lane, schedule a consistency check.
// Right before committing, we will walk the tree and check if any of the
Expand Down Expand Up @@ -8588,11 +8602,11 @@ function rerenderState(initialState) {
return rerenderReducer(basicStateReducer);
}

function pushEffect(tag, create, destroy, deps) {
function pushEffect(tag, create, inst, deps) {
var effect = {
tag: tag,
create: create,
destroy: destroy,
inst: inst,
deps: deps,
// Circular
next: null
Expand All @@ -8619,6 +8633,12 @@ function pushEffect(tag, create, destroy, deps) {
return effect;
}

function createEffectInstance() {
return {
destroy: undefined
};
}

var stackContainsErrorMessage = null;

function getCallerStackFrame() {
Expand Down Expand Up @@ -8719,25 +8739,24 @@ function mountEffectImpl(fiberFlags, hookFlags, create, deps) {
hook.memoizedState = pushEffect(
HasEffect | hookFlags,
create,
undefined,
createEffectInstance(),
nextDeps
);
}

function updateEffectImpl(fiberFlags, hookFlags, create, deps) {
var hook = updateWorkInProgressHook();
var nextDeps = deps === undefined ? null : deps;
var destroy = undefined; // currentHook is null when rerendering after a render phase state update.
var effect = hook.memoizedState;
var inst = effect.inst; // currentHook is null when rerendering after a render phase state update.

if (currentHook !== null) {
var prevEffect = currentHook.memoizedState;
destroy = prevEffect.destroy;

if (nextDeps !== null) {
var prevEffect = currentHook.memoizedState;
var prevDeps = prevEffect.deps;

if (areHookInputsEqual(nextDeps, prevDeps)) {
hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps);
hook.memoizedState = pushEffect(hookFlags, create, inst, nextDeps);
return;
}
}
Expand All @@ -8747,7 +8766,7 @@ function updateEffectImpl(fiberFlags, hookFlags, create, deps) {
hook.memoizedState = pushEffect(
HasEffect | hookFlags,
create,
destroy,
inst,
nextDeps
);
}
Expand Down Expand Up @@ -18963,10 +18982,12 @@ function commitHookEffectListUnmount(
do {
if ((effect.tag & flags) === flags) {
// Unmount
var destroy = effect.destroy;
effect.destroy = undefined;
var inst = effect.inst;
var destroy = inst.destroy;

if (destroy !== undefined) {
inst.destroy = undefined;

if (enableSchedulingProfiler) {
if ((flags & Passive) !== NoFlags) {
markComponentPassiveEffectUnmountStarted(finishedWork);
Expand Down Expand Up @@ -19030,7 +19051,9 @@ function commitHookEffectListMount(flags, finishedWork) {
}
}

effect.destroy = create();
var inst = effect.inst;
var destroy = create();
inst.destroy = destroy;

{
if ((flags & Insertion) !== NoFlags) {
Expand All @@ -19047,8 +19070,6 @@ function commitHookEffectListMount(flags, finishedWork) {
}

{
var destroy = effect.destroy;

if (destroy !== undefined && typeof destroy !== "function") {
var hookName = void 0;

Expand Down Expand Up @@ -20432,12 +20453,13 @@ function commitDeletionEffectsOnFiber(
var effect = firstEffect;

do {
var _effect = effect,
destroy = _effect.destroy,
tag = _effect.tag;
var tag = effect.tag;
var inst = effect.inst;
var destroy = inst.destroy;

if (destroy !== undefined) {
if ((tag & Insertion) !== NoFlags) {
inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
Expand All @@ -20450,13 +20472,15 @@ function commitDeletionEffectsOnFiber(

if (shouldProfile(deletedFiber)) {
startLayoutEffectTimer();
inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy
);
recordLayoutEffectDuration(deletedFiber);
} else {
inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
Expand Down
Loading

0 comments on commit 1f49af9

Please sign in to comment.