Skip to content
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: make .setup-cache.bin in node_modules more reproducible #24480

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

zebreus
Copy link
Contributor

@zebreus zebreus commented Jul 9, 2024

This PR will make vendored dependencies more deterministic. The node_modules/.deno/.setup-cache.bin is currently not deterministic because it consists of serialized HashMaps which have a random order of elements. By switching them out for BTreeMaps, which are sorted in memory, deno generates the same file every time.

According to the Rust documentation BTreeMap might provide slightly less performance than HashMap, but I wasn't able to reproduce lower performance in deno. I think even for large projects, our maps are way too small for this to have any performance impact.

related issue: #24479

@CLAassistant
Copy link

CLAassistant commented Jul 9, 2024

CLA assistant check
All committers have signed the CLA.

@lucacasonato
Copy link
Member

Can you add a comment so that future editors of this code don't switch it back to a HashMap?

@dsherret dsherret changed the title feat: Make .setup-cache.bin more reproducible fix: make .setup-cache.bin in node_modules more reproducible Jul 9, 2024
@dsherret dsherret added this to the 1.45 milestone Jul 9, 2024
The .setup-cache.bin is currently not reproducible because it consists of serialized Hashmaps which have a random order of elements. By switching them out for BTreeMaps, which are always sorted in memory, deno generates the same file every time.
@zebreus
Copy link
Contributor Author

zebreus commented Jul 9, 2024

@lucacasonato @dsherret I added a comment to both changed datastructures.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM

@dsherret dsherret enabled auto-merge (squash) July 9, 2024 17:41
@dsherret dsherret merged commit 3c91d59 into denoland:main Jul 9, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants