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.

Sign up for GitHub

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

Jump to bottom

Possible to shallowcopy a HashMap #55

Closed
longbinlai opened this issue May 23, 2020 · 1 comment · Fixed by #83
Closed

Possible to shallowcopy a HashMap #55

longbinlai opened this issue May 23, 2020 · 1 comment · Fixed by #83

Comments

@longbinlai
Copy link

Thank you for this work. We are recently in using evmap in our DB system as the caching module, but we find out a case that need to use a HashMap as the Value. However, according to the trait bound, the HashMap needs to be ShallowCopiable, which is not yet implemented in the codebase. Is it possible to make it so? Thank you.
@jonhoo
Copy link
Owner

jonhoo commented May 23, 2020

Huh, that is an interesting question... I'm guessing you mean ReadHandle specifically? Since ReadHandle isn't Sync, I doubt you'd be able to use it much even if it implemented ShallowCopy.

Or are you referring to the standard library HashMap? For that one, I think we probably could implement ShallowCopy, although it'll probably be a little involved since it has a more complex inner structure. It should "just work" if you have a Box<HashMap> instead though?

jonhoo added a commit that referenced this issue Nov 29, 2020
@jonhoo
Implement sound map value aliasing
4e83da0 
This implementation gets rid of the unsound `ShallowCopy` trait (#74 &
rust-lang/unsafe-code-guidelines#35 (comment)),
and replaces it with a wrapper type around aliased values.

The core mechanism is that the wrapper type holds a `MaybeUninit<T>`,
and aliases it by doing a `ptr::read` of the whole `MaybeUninit<T>` to
alias. It then takes care to only give out `&T`s, which is allowed to
alias. To drop, the implementation sets a thread-local that the wrapper
uses to determine if it should truly drop the inner `T` (i.e., to
indicate that this is the last copy, and the `T` is no longer aliased).

The removal of `ShallowCopy` makes some of the API nicer, and obviates
the need for `evmap-derive`. Unfortunately the introduction of the
wrapper also complicates certain bounds which now need to know about the
wrapping. Overall though, an ergonomic win.

The implementation passes all tests, and I _think_ it is sound (if you
think it's not, please let me know!). The one bit that I'm not sure
about is the thread-local if a user nests `evmap`s (since they'll share
that thread local).

Note that this does _not_ take care of #78.

Fixes #74.
Fixes #55 since `ShallowCopy` is no longer neeeded.
@jonhoo jonhoo mentioned this issue Nov 29, 2020
Merged
jonhoo added a commit that referenced this issue Dec 6, 2020
@jonhoo
Implement sound map value aliasing
f53e0ee 
This implementation gets rid of the unsound `ShallowCopy` trait (#74 &
rust-lang/unsafe-code-guidelines#35 (comment)),
and replaces it with a wrapper type around aliased values.

The core mechanism is that the wrapper type holds a `MaybeUninit<T>`,
and aliases it by doing a `ptr::read` of the whole `MaybeUninit<T>` to
alias. It then takes care to only give out `&T`s, which is allowed to
alias.

To drop, the implementation casts between two different generic
arguments of the new `Aliased` type, where one type causes the
`Aliased<T>` to drop the inner `T` and the other does not. The
documentation goes into more detail. The resulting cast _should_ be safe
(unlike the old `ManuallyDrop` cast).

The removal of `ShallowCopy` makes some of the API nicer by removing
trait bounds, and obviates the need for `evmap-derive`. While I was
going through, I also took the liberty of tidying up the external API of
`evmap` a bit.

The implementation passes all tests, and I _think_ it is sound (if you
think it's not, please let me know!).

Note that this does _not_ take care of #78.

Fixes #74.
Fixes #55 since `ShallowCopy` is no longer neeeded.
Also fixes #72 for the same reason.
@jonhoo jonhoo closed this as completed in 416ccef Dec 6, 2020
@jonhoo jonhoo closed this as completed in #83 Dec 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement sound map value aliasing jonhoo/left-right
2 participants
@jonhoo @longbinlai