indexmap-rs / indexmap

A hash table with consistent order and fast iteration; access items by key or sequence index
https://docs.rs/indexmap/
Other
1.67k stars 150 forks source link

Expanding mut key API #334

Closed epage closed 1 month ago

epage commented 1 month ago

toml_edit presents Table as a mapping between Key and Value with KeyMut for providing access to the subset of key functionality that the user can modify. Internally, this is stored as IndexMap<String, (Key, Value)>, where Key contains a String, among other data.

Key.get().clone() to get the String key is showing up in my benchmarks in parsing a real-world Cargo.toml file. I'm exploring my options for avoiding that

I hadn't noticed MutableKeys before and it seems like a good start by allowing me to instead do IndexMap<Key, Value>. Some parts I'm missing for maintaining my KeyMut access for users:

There may be more as I've not fully gone through my compilation errors. I'm more wondering if this direction is viable before I continue down this path.

I'd be willing to contribute these if there is interest.

cuviper commented 1 month ago

Key.get().clone() to get the String key is showing up in my benchmarks in parsing a real-world Cargo.toml file. I'm exploring my options for avoiding that

Can you point to the code that's cloning? Maybe I'll have some other suggestions, in case this is an XY problem. For example, implementing Borrow or Equivalent might be enough for simple lookups, or RawEntryApiV1 can handle more advanced cases. Or is the problem just the fundamental duplication between the String key and Key in the value?

Anyway, since KeyMut doesn't allow modifying the Eq/Hash-relevant fields AFAICT, it does seem like an appropriate use case for MutableKeys.

  • MutableKeys::iter_full_mut2

Probably should be iter_mut2? We don't have a "full" regular iterator, but you can enumerate if needed. I guess this would return an IterMut2 with Item = (&mut K, &mut V).

  • MutableOccupiedEntrykeys::get_full_mut2

We don't have any "full" entry methods either, although we could. But just as a parallel to fn key(&self) -> &K, I think one extension trait could be implemented by everyone: Entry, OccupiedEntry, VacantEntry, and even IndexedEntry:

pub trait MutableEntryKey {
    type Key;
    fn key_mut(&mut self) -> &mut Self::Key;
}
epage commented 1 month ago

Can you point to the code that's cloning? Maybe I'll have some other suggestions, in case this is an XY problem. For example, implementing Borrow or Equivalent might be enough for simple lookups, or RawEntryApiV1 can handle more advanced cases. Or is the problem just the fundamental duplication between the String key and Key in the value?

Its fundamental duplication. Its not just to satisfy a lookup but we store both copies. We present a Tables key as a Key which contains both the hashable, ord parts (String) and the mutable format-preserved content of a TOML document. Because this is mutable (and I wasn't aware of the mutable keys API), I duplicate the String inside of the Key for use in HashMap.

Alternatively, I could

example data type API

Probably should be iter_mut2? We don't have a "full" regular iterator, but you can enumerate if needed. I guess this would return an IterMut2 with Item = (&mut K, &mut V).

. But just as a parallel to fn key(&self) -> &K, I think one extension trait could be implemented by everyone: Entry, OccupiedEntry, VacantEntry, and even IndexedEntry:

If those are acceptable paths forward, I can look into adding those on Monday.