-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix #2192 - dont recycle frozen objects #2193
Conversation
b43b3c9
to
3a29865
Compare
I'm curious what the implications of removing the freeze logic entirely from Relay Modern is since its appears to only be applied when |
@dbslone - I hadn't realized that freezing was only done in dev (which makes sense). I think perhaps the right solution here is to flag the object as "scalar" when it is created, and check for that here instead. We could just use a Out of a desire to get this fixed ASAP, I'll submit another PR for that approach right now to replace this one. Apologies to the relay team for the noise! |
Actually, I realized that:
So, I'm re-opening this :) |
Any status on this? Will it be merged soon? |
Ya, we're running a fork locally, since this pretty much blocks development (although it works fine in production). I can't imagine this change would be problematic... |
@jstejada @kassens Would a Relay contributor be willing to look / merge this? This is a blocker for us as well (we use a JSON scalar). Also PRs #2142 & #1868 are really important for me and my team. Some communication on why they haven't been commented in 3+ months would be really helpful. There hasn't been a lot of communication about FB's internal priorities & open source strategy when it comes to Relay. |
I agree with @ekosz, the lack of clear roadmap of the project has been a big issue since Relay was open sourced. There are no clear milestones or time dates of features being released. I am great proponent of Relay and it pains me to see such bad communication with the community. I hope this changes in the future. |
@ekosz I don’t know the answer to that, but maybe some added test coverage could help reviewing? |
@dbslone @mike-marcacci I’m not entirely sure I followed everything correct, but should #2051 be closed in favour of this one? |
@alloy I will close my PR in favor of continuing the discussion in this thread |
Thanks @dbslone 🙏🏽 |
@mike-marcacci Alright, so could you add a test that specifically demonstrates that? |
Sure thing, I’ll add a couple to the PR tonight. |
3a29865
to
6acdeaf
Compare
I've added 3 tests, all of which demonstrate the problem (they fail on the current master branch) and the fix (pass on the PR). |
6acdeaf
to
7bbd68e
Compare
Nice 👌 @jstejada This looks good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jstejada has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good @mike-marcacci. Just added a quick question
@@ -30,29 +30,31 @@ function recycleNodesInto<T>(prevData: T, nextData: T): T { | |||
const prevArray = Array.isArray(prevData) ? prevData : null; | |||
const nextArray = Array.isArray(nextData) ? nextData : null; | |||
if (prevArray && nextArray) { | |||
const isFrozen = Object.isFrozen(nextArray); | |||
canRecycle = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something, but would it make sense to just assign canRecycle
to false
if it is frozen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jstejada, that's a good question. I would not want to prematurely decide that canRecycle
is false
, since it's still possible for wasEqual && nextValue === prevArray[ii]
to be true, and we wouldn't want to lose out on the ability to recycle deep objects just because it is frozen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, thanks for the explanation!
}, true) && prevArray.length === nextArray.length; | ||
} else if (!prevArray && !nextArray) { | ||
// Assign local variables to preserve Flow type refinement. | ||
const prevObject = prevData; | ||
const nextObject = nextData; | ||
const prevKeys = Object.keys(prevObject); | ||
const nextKeys = Object.keys(nextObject); | ||
const isFrozen = Object.isFrozen(nextObject); | ||
canRecycle = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here?
@@ -30,29 +30,31 @@ function recycleNodesInto<T>(prevData: T, nextData: T): T { | |||
const prevArray = Array.isArray(prevData) ? prevData : null; | |||
const nextArray = Array.isArray(nextData) ? nextData : null; | |||
if (prevArray && nextArray) { | |||
const isFrozen = Object.isFrozen(nextArray); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we only freeze in DEV, perhaps we should only perform this isFrozen check in dev?
cc @leebyron
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, I thought about that too but figured isFrozen should be pretty darn quick, and there’s no reason to try to modify a frozen object ever. Happy to add that though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I can go ahead and add that tiny change before landing this PR if that sounds good to you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, not sure if you already made the change over at FB, but I added the dev check here. Let me know if there's anything more I can do to help get this merged...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there! Sorry for the delay (I was off the grid for the last few days). I'll go ahead and add that other test; hopefully my other comments make sense! Also, for what it's worth, I totally don't have any hangups about getting my PR accepted, so if somebody makes a preferred fix on your internal codebase, please feel free to merge that one right away 😊
const prevData = [{foo: 1}, {bar: 2}]; | ||
const nextData = [{foo: 1}, {bar: 2}]; | ||
Object.freeze(nextData); | ||
expect(recycleNodesInto(prevData, nextData)).toBe(prevData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm pretty sure this test is correct. It's testing that even if objects are frozen, we still recycle prevData
object if it is deeply equal to nextData
. This is important for preventing identical renders from triggering react diffing downstream.
The behavior of frozen objects differs from mutable ones only in that we can't preserve object equity in unchanged branches of a state tree that does have changes.
Given this prevData
:
A1
B1 B2
C1 C2 C3 C4
we can recycle the entire object if nextData
looks like this:
A1
B1 B2
C1 C2 C3 C4
...whether or not the anything is frozen.
However, if our nextData
looks like this:
A2
B1 B3
C1 C2 C3 C5
...we can recycle B1 from prevData
, but this requires mutating A2, which crashes the whole app if A2 is frozen. If we were really trying to optimize the frozen object case, it would be possible to create a new A2 to hold the prevData
B1 and nextData
B2, but since this only effects the dev environment, that's optimization that is not worth doing.
const nextData = [{foo: 1}, {bar: 2}]; | ||
Object.freeze(nextData); | ||
expect(recycleNodesInto(prevData, nextData)).toBe(prevData); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing!
@@ -30,29 +30,33 @@ function recycleNodesInto<T>(prevData: T, nextData: T): T { | |||
const prevArray = Array.isArray(prevData) ? prevData : null; | |||
const nextArray = Array.isArray(nextData) ? nextData : null; | |||
if (prevArray && nextArray) { | |||
const isFrozen = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way is fine by me! Am I correct in assuming that these dev flags get replaced with constants in the build process, and then all constant paths get compiled out? If so, then it really doesn't matter – you'll end up with no checks in production regardless of where they happen in dev.
I was thinking about this again (encountered this in another dev environment this morning) and just wanted to clarify one thing from my explanation above, which supports the argument that these In the second example, we CAN and DO recycle B1 from |
As I just commented on your review comment, the test you requested already exists, so I believe this PR is good to go. I've switched my I'm sorry to be pushy, but this is a tremendous headache for me. Every time anyone runs Please let me know if there's anything else I can do to help. |
79aa8db
to
0c8667e
Compare
I cleaned up 3 tests and rebased from master to clean up the commit history, but it looks like doing so pulled in some breakage from master. I'll rebase again once master is passing. Apologies for the extra notifications. |
0c8667e
to
e8e788c
Compare
e8e788c
to
4561712
Compare
Can we get another look here? This is super annoying to have to deal with. |
Hey @jstejada when you get a chance would you mind looking over this again in the context of my above comments? |
@jstejada Any updates on when this will be merged? |
hey @mike-marcacci, sorry for the delay in getting back to this. I'll take a look at your updated changes and comments soon. Thanks for taking the time to update this! |
For anybody else using my fork, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jstejada has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Just checking in … is there a plan to merge this PR? I'm hitting this for doing subscriptions of a Json scalar type. I imagine a lot of people are hitting this, no? |
@mike-marcacci Wanted to test your fork … is that repo public? Couldn't find it. |
Hey @virtualirfan, I published it to NPM as I didn't expect this to take so long, so I didn't bother to do a neat fork and change the readme/metadata/etc from relay's to mine. It's literally just |
Just wanted to report that the latest @jstejada a validation point that might help you pull this PR into the next release. module.exports = {
...
resolve: {
...
alias: {
...
'relay-runtime': '@boltline/relay-runtime',
}
}
} |
Hey @jstejada what is the status on this being merged? |
@jstejada any update on when this PR will be merged? Is the conflict with the test file all thats holding it up? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jstejada has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
What is the status of this, it has been a year? |
The discussion internally is around potential performance impact. We're checking the best way to structure the code such that there is little-to-no overhead in production builds. |
@josephsavona there can't possibly be a performance impact in production builds because all the changes are stripped during the production build process, as mentioned above. |
@mike-marcacci In an ideal world yes, constant propagation and dead code elimination would remove all of the newly added code in prod builds (including the extra if conditions). But that depends on what optimizations are being applied; we’re checking how this affects internal builds in different environments. |
@josephsavona, ah that makes sense. I didn't realize that FB utilized multiple build systems internally; that seems like quite a challenge to keep track of! I will say that while I know this is a particularly hot piece of code, the fact that You made me curious about the possible implications, so I wrote a quick benchmark to make sure my suspicions were correct. Perhaps this will be helpful. :) |
This is one possible fix for #2192, which simply checks if objects are frozen before attempting to modify them.
Do note that while this fixes my use case and any others that are serialized as simple JSON-compatible types, this won't fix cases where a custom scalar is hydrated to a more complex type, such as a
Date
object. For this, you may want to consider adding a check to skip the recycle if the inputs aren't simple objects or arrays:But, since this may have more consequences, and I would really like my use case fixed ASAP 😜 I've omitted that "fix" from this PR. Please let me know if you'd like me to open a separate issue for that.