Conflict between type of HashMap in PipelineCompilationOptions and HashMap exposed by the surface API

[dasifefe]

Description

Currently, the type PipelineCompilationOptions has a field constants of type that is directly defined as hashbrown::HashMap.

However, that type is not exposed to the surface API, making it impossible for the user to declare an object of the same type and then pass to constants (unless I am missing how this type is re-exported from the WGPU crate).

Using [FashHashMap](wgpu::naga::FastHashMap) (which is an alias of hashbrown::HashMap) is also an error, as it uses a hasher different than the one that constants expects. Bellow is the case of using wgpu::naga::FastHashMap<>:

let mut vertex_hash_map_constant: wgpu::naga::FastHashMap<String, f64> = Default::default();
for /* ... */ {
    wgpu::naga::FastHashMap::insert(
        // ...
    );
}
let vertex_wgpu_pipeline_compilation_options = wgpu::PipelineCompilationOptions {
    constants: &vertex_hash_map_constant,
    // ...
};

Error (minus personal information in the output):

error[E0308]: mismatched types
    |
126 |                 constants: &vertex_hash_map_constant,
    |                            ^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&HashMap<String, f64>`, found `&HashMap<String, f64, BuildHasherDefault<FxHasher>>`
    |
    = note: expected reference `&hashbrown::map::HashMap<_, _, foldhash::seed::fast::RandomState>`
               found reference `&hashbrown::map::HashMap<_, _, BuildHasherDefault<rustc_hash::FxHasher>>`

Expected vs observed behavior
Either the hashmap type should be exposed in a way that will not cause confusion to the user, or PipelineCompilationOptions should use a generic type that then WGPU converts internally in whatever it uses.

Platform

WGPU version: 65d499f

[cwfitzgerald]

We really should just use a list hear, as much as I am not thrilled with it.

[dasifefe]

Update

Using let mut options = wgpu::PipelineCompilationOptions::default() to then modify the hashmap of the field constants is not a circumvention to the problem, as the field constants is an immutable reference.

The solution for now is using the latest commit [fcbadc9] before the expanded use of hashbrown [df54acc] in the crate.

[cwfitzgerald]

If you're up to it (or anyone else) this would be a pretty low lift issue to switch to lists.

[dasifefe]

What list type should be used?
wgpu::naga::FastHashMap<>?

[cwfitzgerald]

Just a &[(Key, Value)] type deal

[dasifefe]

I think &[(String, f64)] ended up being a difficult option. That field .constants is passed in multiple places wrapped in a Cow::Borrowed(), which I assume it is for performance reasons. And those places also use a hashmap, so it would either altering a lot of code in the codebase to use Cow<[(String, f64)]> or converting the [(String, f64)] into a hashmap as soon as .constants is used. I think that you guys do not want that, because that would would add unnecessary overhead and defeat the performance reasons for the use of Cow<>.

Looking for another solution

So, I investigated a little the hashmaps used in the codebase:

1. An alias FastHashMap
   

   wgpu/wgpu-core/src/hash_utils.rs

   Lines 6 to 7 in 4e7d892

   	pub type FastHashMap<K, V> =
   	hashbrown::HashMap<K, V, std::hash::BuildHasherDefault<rustc_hash::FxHasher>>;

2. The hashbrown::HashMap is imported in many places, which is the one used by the PipelineCompilationOptions, but not declared as public, which led to the problem in the first place.

3. Also, there is an alias PipelineConstants
   

   wgpu/naga/src/back/mod.rs

   Line 58 in 4e7d892

   pub type PipelineConstants = hashbrown::HashMap<String, f64>;

The alias PipelineConstants to the private hashbrown::HashMap is public, so I used that for the PipelineCompilationOptions.constants and it worked.

Solution:

pub struct PipelineCompilationOptions<'a> {
    pub constants: &'a naga::back::PipelineConstants,
    ...
}

Final questions

1. Currently, we have to use the full path wgpu::naga::back::PipelineConstants in the end-user codebase to use that alias. Should we re-export that alias with a shorter path?

2. Would not be simpler to have only one hashmap type for the whole crate?
   Those two types of hashbrown::HashMap with different hashers make them incompatible with one another.

Sorry about the wall of text.

[cwfitzgerald]

I'll respond to the whole wall of text tomorrow, but I would note that I would not treat the boilerplate of pipeline creation as hot code. Some small waste there is acceptable. I'm not worried about allocating a hashmap here interally.