-
Notifications
You must be signed in to change notification settings - Fork 7
Compute changed keys in pull and mutate #341
Conversation
We now return the touched keys in subscribe queries. We also returned the changed keys in mutation commits and maybe end pull. Towards rocicorp/replicache#63
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a few rounds of polish review but I think it is ready to start looking at
src/prolly/map.rs
Outdated
} | ||
|
||
/// Returns the keys that are different between two maps. | ||
pub fn diff<'a>(a: &'a Self, b: &'a Self) -> Result<Vec<String>, FromUtf8Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One optimization I didn't do is to create key ranges of this... we can do that later.
🎉 One question right off:
It seems like the JS would already have the information it needs here? It know what keys are being requested and the scan parameters. |
That is true. My reason for moving this to Rust was code reuse in future non JS SDK but maybe that was a bad call? At one point I even wanted to keep track of the subscriptions in Rust but due to multiple tabs I moved the storage up to js. |
On Thu, Apr 22, 2021 at 11:12 AM Erik Arvidsson ***@***.***> wrote:
It seems like the JS would already have the information it needs here? It
know what keys are being requested and the scan parameters.
That is true. My reason for moving this to Rust was code reuse in future
non JS SDK but maybe that was a bad call? At one point I even wanted to
keep track of the subscriptions in Rust but due to multiple tabs I moved
the storage up to js.
It's up to you. If you find it easy enough to work in Rust, I don't care. I
just don't want hypothetical desire to work on other platforms in the
future to slow us down at all now. If you find yourself fighting with rust,
and it's easy to move it out to JS, do it.
… —
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#341 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAATUBFXCOVDYQPTKO4G4FTTKCGMLANCNFSM43NDQ25A>
.
|
Doing it in JS to see how much complexity it removes. |
Some(pending_val) => match self.base_get(key) { | ||
Some(base_val) => { | ||
if pending_val != base_val { | ||
keys.push(String::from_utf8(key.clone())?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do the conversion to utf8 here?
prolly::Map keys are [u8]
so I would expect its diff API to be expressed in those terms too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sad to copy/allocate all the strings. Did you consider attempt returning references to the data already in the map? Not sure if it's worth the complexity...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be copied later so I copied it earlier since it made the code simpler. I can think a bit more about this to see if it makes sense to go [u8]
all the way to JS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to do this in another PR
src/db/read.rs
Outdated
} | ||
} | ||
|
||
pub type Subscription = Arc<RwLock<SubscriptionInner>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need concurrent mutability of this data? I'm very concerned about the proliferation of locks in our code as it will made deadlock increasingly difficult to avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory we do not need locks because JS and Wasm is single threaded but I don't think Rust would allow that.
The following case needs "concurrent" access.
rep.subscribe(tx =>
Promise.all([tx.get('a'), tx.get('b')])
);
I do not see how this could deadlock but it is always a good thing to keep in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least some of the proliferation of locks in the rust code is due to rust not allowing inner mutability without them. Where in another language we wouldn't bother putting a lock around a thing that we know is not going to be accessed concurrently, rust makes us.
src/sync/pull.rs
Outdated
@@ -1203,6 +1297,7 @@ mod tests { | |||
intervening_sync: false, | |||
exp_replay_ids: vec![], | |||
exp_err: None, | |||
exp_changed_keys: ChangedKeysMap::new(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phritz It seems like this test are all about nop mutations and pulls? I was looking for a rust/wasm test that does pull with changes but I guess we didn't need to test this before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready for realz. PTAL
Some(pending_val) => match self.base_get(key) { | ||
Some(base_val) => { | ||
if pending_val != base_val { | ||
keys.push(String::from_utf8(key.clone())?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to do this in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this a careful reading and it looks great! Only a couple minor comments.
Return the changed keys in mutation commits and maybe end pull.
Towards rocicorp/replicache#63