[Flight] Remove back pointers to the Response from the Chunks #33620
+34
−39
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It's the only one that needs it.
The reason field is free to be reused for this purpose.
|
One thing we could also do is in idle cycles, warm up any remaining lazy references by initializing them. Which could save future CPU work and release memory by figuring out which references will end up being used. |
unstubbable
approved these changes
Jun 23, 2025
| @@ -625,6 +609,7 @@ function initializeModelChunk<T>(chunk: ResolvedModelChunk<T>): void { | |||
| initializingHandler = null; | |||
|
|
|||
| const resolvedModel = chunk.value; | |||
| const response = chunk.reason; | |||
There was a problem hiding this comment.
Thanks, I hate it. ... I vote for evil, by the way. 😉
| @@ -486,31 +472,31 @@ function createResolvedModelChunk<T>( | |||
| value: UninitializedModel, | |||
| ): ResolvedModelChunk<T> { | |||
| // $FlowFixMe[invalid-constructor] Flow doesn't support functions as constructors | |||
| return new ReactPromise(RESOLVED_MODEL, value, null, response); | |||
| return new ReactPromise(RESOLVED_MODEL, value, response); | |||
There was a problem hiding this comment.
A comment like we have for createInitializedStreamChunk would help avoid confusion:
// We use the reason field to stash the controller since we already have that
// field. It's a bit of a hack but efficient.
There was a problem hiding this comment.
It's all over every access in this file and indeed every Fiber field too. At some point you just got to add a doc for the whole repo.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This frees some memory that will be even more important in a follow up.
Currently, all
ReactPromiseinstances hold onto their originalResponse. TheResponseholds onto all objects that were in that response since they're needed in case the parsed content ends up referring to an existing object. If everything you retain are plain objects then that's fine and theResponsegets GC:ed, but if you're retaining aPromiseitself then it holds onto the wholeResponse.The only thing that needs this reference at all is a
ResolvedModelChunksince it will lazily initialize e.g. by calling.thenon itself and so we need to know where to find any sibling chunks it may refer to. However, we can just store theResponseon thereasonfield for this particular state.That way when all lazy values are touched and initialized the
Responseis freed. We also free up some memory by getting rid of the extra field.