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

Implement HashStable for StableMap/StableSet instead of HashMap/HashSet #84446

Open
Aaron1011 opened this issue Apr 22, 2021 · 3 comments
Open
Labels
A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Apr 22, 2021

The implementation of HashStable for HashSet uses ToStableHashKey to sort the entries before hashing:

pub fn hash_stable_hashmap<HCX, K, V, R, SK, F>(
hcx: &mut HCX,
hasher: &mut StableHasher,
map: &::std::collections::HashMap<K, V, R>,
to_stable_hash_key: F,
) where
K: Eq,
V: HashStable<HCX>,
R: BuildHasher,
SK: HashStable<HCX> + Ord,
F: Fn(&K, &HCX) -> SK,
{
let mut entries: Vec<_> = map.iter().map(|(k, v)| (to_stable_hash_key(k, hcx), v)).collect();
entries.sort_unstable_by(|&(ref sk1, _), &(ref sk2, _)| sk1.cmp(sk2));
entries.hash_stable(hcx, hasher);
}

However, nothing ensures that a HashMap is used in an order-independent way by other code. In particular, a query could directly iterate over the entires, and produce different results for two HashMaps that have the same Fingerprint.

We should enforce this order independence by only implementing HashStable on a wrapper type that hides the underlying iteration order. We could probably use StableMap and StableSet, but we'll need the ability to customize the key used by into_sorted_vector (we'll want to sort using a stable key, not the Ord implementation of a DefId).

@Aaron1011 Aaron1011 added A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 22, 2021
@jyn514 jyn514 self-assigned this Apr 22, 2021
@jyn514
Copy link
Member

jyn514 commented Apr 22, 2021

@Aaron1011 can I add a StableSet::any(f: impl Fn() -> bool) -> bool function? Or does that defeat the point because it can observe the iteration order? This code seems a little silly:

        for id in defids.to_sorted_vec() {
            let CodegenFnAttrs { optimize, .. } = tcx.codegen_fn_attrs(*id);
            match optimize {
                attr::OptimizeAttr::None => continue,
                attr::OptimizeAttr::Size => continue,
                attr::OptimizeAttr::Speed => {
                    return for_speed;
                }
            }
        }

@jyn514
Copy link
Member

jyn514 commented Apr 22, 2021

Another piece of code that seems silly:

        self.typeck_results.add_coercion_casts(fcx_coercion_casts.to_sorted_vec());

coercion_casts is just a normal hashmap, the insertion order doesn't matter.

@jyn514 jyn514 removed their assignment Apr 22, 2021
jyn514 added a commit to jyn514/rust that referenced this issue Apr 22, 2021
@jyn514
Copy link
Member

jyn514 commented Apr 22, 2021

I am not making much progress on this - I have some broken in changes in jyn514@f7f913c if you want to reuse them, but it might be easier to start from scratch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants