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

WIP: Add views of OrderMap #47

Closed
wants to merge 2 commits into from
Closed

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Nov 30, 2017

OrderMap::view() -> OrderMapView and view_mut() -> OrderMapViewMut
provide access to items in the map, but no ability to add/remove items.
These views can be sub-divided with split_at etc., which may be useful
to enable parallelism.

`OrderMap::view() -> OrderMapView` and `view_mut() -> OrderMapViewMut`
provide access to items in the map, but no ability to add/remove items.
These views can be sub-divided with `split_at` etc., which may be useful
to enable parallelism.
@cuviper
Copy link
Member Author

cuviper commented Nov 30, 2017

Views were suggested here: #45 (comment)

The two views have different contents, so I thought I should explain why, and we can decide how to proceed.

OrderMapView is just a reference to the original map and range indices. Everything is simple.

OrderMapViewMut has to break it down, because it needs exclusive non-overlapping slices of entries in order to be mutable. One notable difference this causes is that key lookup is less efficient. If the hash gets an index from pos that's not in our range, we have to just assume that the key at that remote entry would not be equal, and thus we must continue the linear probing as if it were a hash collision. I'm not sure how much this will matter to performance in practice.

This setup also means that OrderMapViewMut can't degrade to an OrderMapView, which makes split_at awkward -- it ought to take &self and return immutable OrderMapViews.

One idea is that OrderMapView could have the same fields broken out, but hang on to as much of entries as possible -- so it would need the offset and the start..end indexes (end offset is implicit in entries.len()). When created from a map, it would keep referencing the entire entries (offset = 0) with splits only affecting start and end. When created from an OrderMapViewMut, it would have that reduced view of entries. This way OrderMapView keeps as much as it can for key lookup comparisons, but would still work with a limited view too.

@cuviper
Copy link
Member Author

cuviper commented Nov 30, 2017

The other thing I wanted to point out is that sadly I don't think it's possible to allow Index with ranges. That has to return a reference, &Self::Output, and we don't have a nice [T] or str type we can use for Output like this.

@bluss
Copy link
Member

bluss commented Dec 1, 2017

Hey, I'm thrilled! Just love to see an idea tried out and come to life

@bluss
Copy link
Member

bluss commented Dec 1, 2017

Yes the Rust limitations for views is something I know well from ndarray 😄 I also have a preconceived notion from ndarray's view design: it would use .split_at(self, index) -> (Self, Self) for both the view types. You could downgrade explicitly with .view(&self) -> View and reborrow with .view_mut(&mut self) -> ViewMut (but it is never going to equal the slice exactly.).

@cuviper
Copy link
Member Author

cuviper commented Dec 1, 2017

OK, splitting by value works.

if let Some((i, hash_proxy)) = self.indices[probe].resolve::<Sz>() {
// Unfortunately, we can only look at indexes within our slice
// of entries, which might mean we'll keep probing farther than
// it would be strictly necessary.
Copy link
Member

@bluss bluss Dec 29, 2017

Choose a reason for hiding this comment

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

We can remedy this (often); by asking for the short hash that is stored in the indices; it is accessible when usize is 64-bit and the entry capacity fits in 32 bits. It requires breaking down get_short_hash and using something like this:

/// Get the hash from the indices if possible.
fn try_short_hash<K, V>(&self) -> Option<ShortHash<Sz>> {
    if Sz::is_64_bit() {
        None
    } else {
        Some(ShortHash(self.0, PhantomData))
    }
}

@RReverser
Copy link

Is this still in works or decided against in post-OrderMap world? (I wouldn't generally ping on issues/PRs but seems that this was inactive for half a year)

@cuviper
Copy link
Member Author

cuviper commented Jul 18, 2018

I haven't thought about it in a while. Is this a feature you would like?
I should revisit the Rayon PR #45 too...

@bluss
Copy link
Member

bluss commented Jul 24, 2018

@RReverser I haven't had much time for github, that goes up and down sometimes.

@cuviper Right, so how do we decide if this is actually useful or just a ridiculously cool idea :-) I'd say we want this, thanks for all the implementation work but if you want to you can leave it for me to finish it, that's fine with me. (These kinds of view definitions in Rust, they don't feel entirely perfect in general, not sure if we are missing language features or what we are missing to really express this in a better way; having multiple kinds of equivalent read-only IndexMap-ish types then later demands traits that treats the two as equal and so on..)

@RReverser
Copy link

@cuviper Initially I thought I do need this, but then realised it's not what I thought it is :)

@cuviper
Copy link
Member Author

cuviper commented Jun 17, 2022

#177 did this better.

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.

3 participants