implement dict as an Alloc allocated table#29
Conversation
maximecb
left a comment
maximecb
left a comment
There was a problem hiding this comment.
Looks pretty good, you did a good job, linear probing is the right choice 👍
|
Could you also add some tests in |
I reused rust Hash implementation for the hashing algorithm, then I used a simply linear probing strategy to handle collisions. I have set a threshold of 75% for when we double the size of the table.
|
@maximecb sorry it took a while but i finally got hash working under gc as well. I have introduce a |
|
No worries Marco. Thanks for contributing :) I fixed and tested a couple of other GC things during the week. |
src/alloc.rs
Outdated
| let p_str = self.alloc(Str::new(raw_str_ptr))?; | ||
| pub fn str(&mut self, s: &str) -> Result<*const Str, ()> | ||
| { | ||
| let inner = self.raw_str(s)?; | ||
| let p_str = self.alloc(inner)?; |
There was a problem hiding this comment.
Can you rebase the PR? I think there should be no diff in alloc.rs
| #[derive(Clone, Copy)] | ||
| struct TableSlot(Option<(*const Str, Value)>); | ||
|
|
||
| impl TableSlot { | ||
| fn new(key: *const Str, val: Value) -> Self { | ||
| Self(Some((key, val))) | ||
| } |
There was a problem hiding this comment.
I think probably the way I would go about this is use a null ptr for the key when the slot is empty. We could add an is_empty() method.
You can probably get rid of the option type on value and value_mut, just panic if the slot is empty? Use an assert even?
There was a problem hiding this comment.
i think it still makes sense to return an option for the value because it makes implementing values and values_mut with filter_map pretty easy
maximecb
left a comment
maximecb
left a comment
There was a problem hiding this comment.
The implementation looks good 👁️
I think the PR needs to be rebased because there are some diffs that don't match head.
I made some comments wrt TableSlot. I think it shouldn't be too hard to make the refactoring to use a null pointer for the key and it should be much cleaner. Other than that, looks ready to me 👌
|
I merged it and added some tests, fixed some minor bugs. There's one thing that seems inefficient with the current implementation: the string key always gets copied on set, but that key should already be GC-allocated. The keys are also copied when expanding the table, which shouldn't be necessary. |
|
oh yeah good point I'll optimise that tomorrow if I get to it! |
2 participants
I reused rust Hash implementation for the hashing algorithm, then I used
a simply linear probing strategy to handle collisions.
I have set a threshold of 75% for when we double the size of the table.
For some reason I had to zero manually the entries of the table on creation because I was getting some dirty buffers, and some stuff was coming out as Some(...) even though we had just created an empty table.