Skip to content

Compress buildinfo by changing how affectedFilesPendingEmit are stored #51246

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 31, 2022
Merged

Compress buildinfo by changing how affectedFilesPendingEmit are stored #51246

merged 1 commit into from
Oct 31, 2022

Conversation

@sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Oct 20, 2022

A file is stored as just fileId if pending on "Full" emit and as [fileId] if its pending on Dts so this avoids coding BuilderFileEmitKind saving some bytes. Becasue in most cases it will be full emit pending (eg if there are errors) it is used as fileId.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 20, 2022
@sheetalkamat sheetalkamat requested a review from amcasey October 20, 2022 19:01
@amcasey
Copy link
Member

amcasey commented Oct 28, 2022

I think the description basically says that, rather than emitting both a fileId and a BuilderFileEmitKind (presumably, a two-state?) we will use array-ness to encode the flag, thereby producing shorter output? And the shorter of the two encodings (non-array) will be used for the more common case (full emit)? Is that right?

@amcasey
Copy link
Member

amcasey commented Oct 28, 2022 •
edited
Loading

If the flag can differ from file to file (is that actually the case?), then would it be even more compact to just have two lists: files pending full emit and files pending dts emit?

Edit: I found a test showing that it's possible. It's still not clear how much overhead splitting the list would have and it might regress some scenarios (e.g. the (contrived) tests), so we can probably ignore this for now.

amcasey
amcasey approved these changes Oct 28, 2022
View reviewed changes
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see a scenario where this is worse than the status quo, but it does make me wonder whether we should just emit a binary file.

@DanielRosenwasser DanielRosenwasser added the Merge/Review for Next Release This PR should be re-reviewed and merged ASAP for the next release. label Oct 28, 2022
@sheetalkamat sheetalkamat merged commit 18f559f into main Oct 31, 2022
@sheetalkamat sheetalkamat deleted the dtsPending branch October 31, 2022 17:14
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 5.0.0 milestone Oct 31, 2022
@DanielRosenwasser DanielRosenwasser mentioned this pull request Oct 31, 2022
Closed
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@amcasey amcasey amcasey approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sheetalkamat sheetalkamat

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug Merge/Review for Next Release This PR should be re-reviewed and merged ASAP for the next release.

Projects

None yet

Milestone

TypeScript 5.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@sheetalkamat @amcasey @DanielRosenwasser @typescript-bot