Skip to content

Commit

Permalink
refactor: re-implement map type
Browse files Browse the repository at this point in the history
  • Loading branch information
forehalo committed Sep 15, 2023
1 parent e74a132 commit 1edc2b8
Show file tree
Hide file tree
Showing 17 changed files with 380 additions and 379 deletions.
6 changes: 3 additions & 3 deletions y-octo/benches/map_ops_benchmarks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ fn operations(c: &mut Criterion) {
let doc = Doc::default();
let mut map = doc.get_or_create_map("test").unwrap();
for (idx, key) in base_text.iter().enumerate() {
map.insert(key, idx).unwrap();
map.insert(key.to_string(), idx).unwrap();
}
});
});
Expand Down Expand Up @@ -49,7 +49,7 @@ fn operations(c: &mut Criterion) {
let doc = Doc::default();
let mut map = doc.get_or_create_map("test").unwrap();
for (idx, key) in base_text.iter().enumerate() {
map.insert(key, idx).unwrap();
map.insert(key.to_string(), idx).unwrap();
}

b.iter(|| {
Expand Down Expand Up @@ -93,7 +93,7 @@ fn operations(c: &mut Criterion) {
let doc = Doc::default();
let mut map = doc.get_or_create_map("test").unwrap();
for (idx, key) in base_text.iter().enumerate() {
map.insert(key, idx).unwrap();
map.insert(key.to_string(), idx).unwrap();
}
for key in &base_text {
map.remove(key);
Expand Down
2 changes: 1 addition & 1 deletion y-octo/bin/memory_leak_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ fn run_map_test() {
let doc = Doc::default();
let mut map = doc.get_or_create_map("test").unwrap();
for (idx, key) in base_text.iter().enumerate() {
map.insert(key, idx).unwrap();
map.insert(key.to_string(), idx).unwrap();
}
}
}
Expand Down
23 changes: 10 additions & 13 deletions y-octo/src/doc/codec/item.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::*;
use crate::sync::{Arc, AtomicU8, Ordering};
use crate::sync::{AtomicU8, Ordering};

#[derive(Debug, Clone)]
#[cfg_attr(test, derive(proptest_derive::Arbitrary))]
Expand Down Expand Up @@ -118,16 +118,13 @@ pub(crate) struct Item {
pub id: Id,
pub origin_left_id: Option<Id>,
pub origin_right_id: Option<Id>,
#[cfg_attr(all(test, not(loom)), proptest(value = "Somr::default()"))]
pub left: Somr<Item>,
#[cfg_attr(all(test, not(loom)), proptest(value = "Somr::default()"))]
pub right: Somr<Item>,
#[cfg_attr(all(test, not(loom)), proptest(value = "Somr::none()"))]
pub left: ItemRef,
#[cfg_attr(all(test, not(loom)), proptest(value = "Somr::none()"))]
pub right: ItemRef,
pub parent: Option<Parent>,
pub parent_sub: Option<String>,
// make content Arc, so we can share the content between items
// and item can be readonly and cloned fast.
// TODO: considering using Cow
pub content: Arc<Content>,
pub content: Content,
#[cfg_attr(all(test, not(loom)), proptest(value = "ItemFlags::default()"))]
pub flags: ItemFlags,
}
Expand Down Expand Up @@ -187,7 +184,7 @@ impl Default for Item {
right: Somr::none(),
parent: None,
parent_sub: None,
content: Arc::new(Content::Deleted(0)),
content: Content::Deleted(0),
flags: ItemFlags::from(0),
}
}
Expand Down Expand Up @@ -216,7 +213,7 @@ impl Item {
right,
parent,
parent_sub,
content: Arc::new(content),
content,
flags,
}
}
Expand Down Expand Up @@ -369,7 +366,7 @@ impl Item {
// tag must not GC or Skip, this must process in parse_struct
debug_assert_ne!(first_5_bit, 0);
debug_assert_ne!(first_5_bit, 10);
Arc::new(Content::read(decoder, first_5_bit)?)
Content::read(decoder, first_5_bit)?
},
left: Somr::none(),
right: Somr::none(),
Expand All @@ -380,7 +377,7 @@ impl Item {
item.flags.set_countable();
}

if matches!(item.content.as_ref(), Content::Deleted(_)) {
if matches!(item.content, Content::Deleted(_)) {
item.flags.set_deleted();
}

Expand Down
6 changes: 2 additions & 4 deletions y-octo/src/doc/codec/refs.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use sync::Arc;

use super::*;

// make fields Copy + Clone without much effort
Expand Down Expand Up @@ -81,7 +79,7 @@ impl Node {
_ => {
let item = Somr::new(Item::read(decoder, id, info, first_5_bit)?);

if let Content::Type(ty) = item.get().unwrap().content.as_ref() {
if let Content::Type(ty) = &item.get().unwrap().content {
if let Some(mut ty) = ty.ty_mut() {
ty.item = item.clone();
}
Expand Down Expand Up @@ -271,7 +269,7 @@ impl Node {
return false;
}

match (Arc::make_mut(&mut litem.content), Arc::make_mut(&mut ritem.content)) {
match (&mut litem.content, &mut ritem.content) {
(Content::Deleted(l), Content::Deleted(r)) => {
*l += *r;
}
Expand Down
5 changes: 2 additions & 3 deletions y-octo/src/doc/codec/utils/items.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use super::{item::item_flags, *};
use crate::sync::Arc;

pub(crate) struct ItemBuilder {
item: Item,
Expand Down Expand Up @@ -54,7 +53,7 @@ impl ItemBuilder {
}

pub fn content(mut self, content: Content) -> ItemBuilder {
self.item.content = Arc::new(content);
self.item.content = content;
self
}

Expand Down Expand Up @@ -92,7 +91,7 @@ mod tests {
assert_eq!(item.origin_right_id, Some(Id::new(4, 5)));
assert!(matches!(item.parent, Some(Parent::String(text)) if text == "test"));
assert_eq!(item.parent_sub, None);
assert_eq!(item.content, Arc::new(Content::Any(vec![Any::String("Hello".into())])));
assert_eq!(item.content, Content::Any(vec![Any::String("Hello".into())]));
});
}
}
2 changes: 1 addition & 1 deletion y-octo/src/doc/document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ mod tests {
let doc = Doc::new();

let mut map = doc.get_or_create_map("abc").unwrap();
map.insert("a", 1).unwrap();
map.insert("a".to_string(), 1).unwrap();
let binary = doc.encode_update_v1().unwrap();

let doc_new = Doc::new();
Expand Down
12 changes: 5 additions & 7 deletions y-octo/src/doc/history.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,7 @@ impl StoreHistory {
histories.push(History {
id: item.id.to_string(),
parent: Self::parse_path(item, &parents),
content: Value::try_from(item.content.as_ref())
.map(|v| v.to_string())
.unwrap_or("unknown".to_owned()),
content: Value::from(&item.content).to_string(),
})
}

Expand Down Expand Up @@ -243,8 +241,8 @@ mod test {
let doc = Doc::default();
let mut map = doc.get_or_create_map("map").unwrap();
let mut sub_map = doc.create_map().unwrap();
map.insert("sub_map", sub_map.clone()).unwrap();
sub_map.insert("key", "value").unwrap();
map.insert("sub_map".to_string(), sub_map.clone()).unwrap();
sub_map.insert("key".to_string(), "value").unwrap();

assert_eq!(doc.clients()[0], doc.client());
});
Expand All @@ -256,8 +254,8 @@ mod test {
let doc = Doc::default();
let mut map = doc.get_or_create_map("map").unwrap();
let mut sub_map = doc.create_map().unwrap();
map.insert("sub_map", sub_map.clone()).unwrap();
sub_map.insert("key", "value").unwrap();
map.insert("sub_map".to_string(), sub_map.clone()).unwrap();
sub_map.insert("key".to_string(), "value").unwrap();

let history = StoreHistory::new(&doc.store);

Expand Down
6 changes: 3 additions & 3 deletions y-octo/src/doc/publisher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,12 +201,12 @@ mod tests {
});

let mut map = doc.get_or_create_map("test").unwrap();
map.insert("key1", "val1").unwrap();
map.insert("key1".to_string(), "val1").unwrap();

sleep(Duration::from_secs(1));

map.insert("key2", "val2").unwrap();
map.insert("key3", "val3").unwrap();
map.insert("key2".to_string(), "val2").unwrap();
map.insert("key3".to_string(), "val3").unwrap();
sleep(Duration::from_secs(1));

let mut array = doc.get_or_create_array("array").unwrap();
Expand Down
43 changes: 13 additions & 30 deletions y-octo/src/doc/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ impl DocStore {
let id = (self.client(), self.get_state(self.client())).into();
let item = Somr::new(Item::new(id, content, left, right, parent, parent_sub));

if let Content::Type(ty) = item.get().unwrap().content.as_ref() {
if let Content::Type(ty) = &item.get().unwrap().content {
if let Some(mut ty) = ty.ty_mut() {
ty.item = item.clone();
}
Expand Down Expand Up @@ -375,15 +375,12 @@ impl DocStore {
Some(Parent::Id(parent_id)) => {
match self.get_node(*parent_id) {
Some(Node::Item(parent_item)) => {
match &parent_item.get().unwrap().content.as_ref() {
Content::Type(ty) => {
item.parent.replace(Parent::Type(ty.clone()));
}
_ => {
// invalid parent, take it.
item.parent.take();
// return Err(JwstCodecError::InvalidParent);
}
if let Content::Type(ty) = &parent_item.get().unwrap().content {
item.parent.replace(Parent::Type(ty.clone()));
} else {
// invalid parent, take it.
item.parent.take();
// return Err(JwstCodecError::InvalidParent);
}
}
_ => {
Expand All @@ -406,7 +403,7 @@ impl DocStore {
};

// assign store in ytype to ensure store exists if a ytype not has any children
if let Content::Type(ty) = Arc::make_mut(&mut item.content) {
if let Content::Type(ty) = &mut item.content {
ty.store = Arc::downgrade(&store_ref);

// we keep ty owner in dangling_types so the delete of any type will not make it
Expand Down Expand Up @@ -450,7 +447,7 @@ impl DocStore {
this.origin_left_id = left_ref.get().map(|left| left.last_id());
this.left = left_ref;
}
this.content = Arc::new(this.content.split(offset)?.1);
this.content = this.content.split(offset)?.1;
}

if let Some(Parent::Type(ty)) = &this.parent {
Expand Down Expand Up @@ -618,7 +615,7 @@ impl DocStore {
}
}

match item.content.as_ref() {
match &item.content {
Content::Type(ty) => {
if let Some(mut ty) = ty.ty_mut() {
// items in ty are all refs, not owned
Expand Down Expand Up @@ -851,7 +848,7 @@ impl DocStore {
let _ = mem::replace(&mut items[idx], Node::new_gc(item.id, item.len()));
} else {
let mut item = unsafe { item_ref.get_mut_unchecked() };
item.content = Arc::new(Content::Deleted(item.len()));
item.content = Content::Deleted(item.len());
item.flags.clear_countable();
debug_assert!(!item.flags.countable());
}
Expand Down Expand Up @@ -1141,14 +1138,7 @@ mod tests {
store.gc_delete_set().unwrap();

assert_eq!(
store
.get_node((1, 0))
.unwrap()
.as_item()
.get()
.unwrap()
.content
.as_ref(),
&store.get_node((1, 0)).unwrap().as_item().get().unwrap().content,
&Content::Deleted(4)
);
});
Expand All @@ -1173,14 +1163,7 @@ mod tests {

assert_eq!(arr.len(), 0);
assert_eq!(
store
.get_node((1, 0))
.unwrap()
.as_item()
.get()
.unwrap()
.content
.as_ref(),
&store.get_node((1, 0)).unwrap().as_item().get().unwrap().content,
&Content::Deleted(1)
);

Expand Down
14 changes: 7 additions & 7 deletions y-octo/src/doc/types/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,16 @@ impl_type!(Array);

impl ListType for Array {}

pub struct ArrayIter(ListIterator);
pub struct ArrayIter<'a>(ListIterator<'a>);

impl Iterator for ArrayIter {
impl Iterator for ArrayIter<'_> {
type Item = Value;

fn next(&mut self) -> Option<Self::Item> {
for item in self.0.by_ref() {
if let Some(item) = item.get() {
if item.countable() {
return Some(item.content.as_ref().try_into().unwrap());
return Some(Value::try_from(&item.content).unwrap());
}
}
}
Expand All @@ -39,9 +39,9 @@ impl Array {
debug_assert!(offset == 0);
if let Some(item) = item.get() {
// TODO: rewrite to content.read(&mut [Any])
return match item.content.as_ref() {
return match &item.content {
Content::Any(any) => return any.first().map(|any| Value::Any(any.clone())),
_ => item.content.as_ref().try_into().map_or_else(|_| None, Some),
_ => Value::try_from(&item.content).map_or_else(|_| None, Some),
};
}

Expand Down Expand Up @@ -83,8 +83,6 @@ impl serde::Serialize for Array {

#[cfg(test)]
mod tests {
use yrs::{Array, Options, Text, Transact};

use super::*;

#[test]
Expand All @@ -108,6 +106,7 @@ mod tests {
#[test]
#[cfg_attr(miri, ignore)]
fn test_ytext_equal() {
use yrs::{Options, Text, Transact};
let options = DocOptions::default();
let yrs_options = Options::default();

Expand Down Expand Up @@ -166,6 +165,7 @@ mod tests {
#[test]
#[cfg_attr(miri, ignore)]
fn test_yrs_array_decode() {
use yrs::{Array, Transact};
let update = {
let doc = yrs::Doc::new();
let array = doc.get_or_insert_array("abc");
Expand Down
11 changes: 3 additions & 8 deletions y-octo/src/doc/types/list/iterator.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use super::*;

pub(crate) struct ListIterator {
pub(crate) struct ListIterator<'a> {
pub(super) _lock: RwLockReadGuard<'a, YType>,
pub(super) cur: Somr<Item>,
}

impl Iterator for ListIterator {
impl Iterator for ListIterator<'_> {
type Item = Somr<Item>;

fn next(&mut self) -> Option<Self::Item> {
Expand All @@ -20,9 +21,3 @@ impl Iterator for ListIterator {
None
}
}

impl ListIterator {
pub fn new(start: Somr<Item>) -> Self {
Self { cur: start }
}
}
5 changes: 4 additions & 1 deletion y-octo/src/doc/types/list/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,10 @@ pub(crate) trait ListType: AsInner<Inner = YTypeRef> {

fn iter_item(&self) -> ListIterator {
let inner = self.as_inner().ty().unwrap();
ListIterator::new(inner.start.clone())
ListIterator {
cur: inner.start.clone(),
_lock: inner,
}
}

fn find_pos(&self, inner: &YType, index: u64) -> Option<ItemPosition> {
Expand Down
Loading

0 comments on commit 1edc2b8

Please sign in to comment.