-
Notifications
You must be signed in to change notification settings - Fork 50.2k
[Flight] Keep a separate ref count for debug chunks #33717
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
Conversation
|
Comparing: 9a645e1...cd60c89 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
unstubbable
left a comment
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.
We also need to change this line, don't we?
| request.pendingChunks--; |
The Postpone changes seem unrelated. Are you starting to clean up already?
| if (debug) { | ||
| request.pendingDebugChunks++; | ||
| } else { | ||
| request.pendingChunks++; // Extra chunk for the header. | ||
| } |
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.
Should we co-locate this with the if/else below?
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.
It might be misleading. It’s unfortunate the way it’s set up now that we add counts separately from pushing the chunks. We have to sometimes since they’re not always resolved immediately.
In this case this increment is not an exact pair with the push like it is in some other places. This is actually just one of two increments. So it might be misleading.
It’s more colocated with the header than the push.
|
I never implemented the postpone ones correctly in the first place for the debug channel so I just removed those branches. Treated as errors. Imminent deletion anyway. |
Fixed. |
Same as #33716 but without the separate close signal.
We'll need the ref count for separate debug channel anyway but I'm not sure we'll need the separate close signal.