Unalign PackedFingerprint on all hosts, not just x86 and x86-64#152710
Open
Zalathar wants to merge 1 commit intorust-lang:mainfrom
Open
Unalign PackedFingerprint on all hosts, not just x86 and x86-64#152710Zalathar wants to merge 1 commit intorust-lang:mainfrom
PackedFingerprint on all hosts, not just x86 and x86-64#152710Zalathar wants to merge 1 commit intorust-lang:mainfrom
Conversation
Collaborator
|
r? @mati865 rustbot has assigned @mati865. Use Why was this reviewer chosen?The reviewer was selected based on:
|
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
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.
Back in #78646,
DepNodewas modified to store an unalignedPackedFingerprintinstead of an 8-byte-alignedFingerprint. That reduced the size of DepNode from 24 bytes to 17 bytes (nowadays 18 bytes), resulting in considerable memory savings in incremental builds.See #152695 (comment) for a benchmark demonstrating the impact of removing that optimization.
At the time (and today), the unaligning was only performed on x86 and x86-64 hosts, because those CPUs are known to generally have low overhead for unaligned memory accesses. Hosts with other CPU architectures would continue to use an 8-byte-aligned fingerprint and a 24-byte DepNode.
Given the subsequent rise of aarch64 (especially on macOS) and other architectures, it's a shame that some commonly-used builds of rustc don't get those memory-size benefits, based on a decision made several years ago under different circumstances.
We don't have benchmarks to show the actual effect of unaligning DepNode fingerprints on various non-x86 hosts, but it seems very likely to be a good idea on Apple chips, and I have no particular reason to think that it will be catastrophically bad on other hosts. And we don't typically perform this kind of speculative pessimization in other parts of the compiler.