summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMatthias Beyer <mail@beyermatthias.de>2018-09-27 13:03:57 +0200
committerGitHub <noreply@github.com>2018-09-27 13:03:57 +0200
commit9bce68b1bff5e2f77f7c86424de5c16a7e38a2cc (patch)
tree3a3c4ef06ce1de433ef76ee599bb914c1d7e27ef
parentc791977aab6eb94520cf299fb05a8355da10718c (diff)
parentd4872f6da3348e990aacfbccb2e09862f710af37 (diff)
downloadimag-9bce68b1bff5e2f77f7c86424de5c16a7e38a2cc.zip
imag-9bce68b1bff5e2f77f7c86424de5c16a7e38a2cc.tar.gz
Merge pull request #1494 from matthiasbeyer/libimagstore/optimize-backend-iterator
Optimize the Store::entries() interface
-rw-r--r--lib/core/libimagstore/src/file_abstraction/fs.rs30
-rw-r--r--lib/core/libimagstore/src/file_abstraction/inmemory.rs23
-rw-r--r--lib/core/libimagstore/src/file_abstraction/iter.rs67
-rw-r--r--lib/core/libimagstore/src/file_abstraction/mod.rs3
-rw-r--r--lib/core/libimagstore/src/iter.rs78
-rw-r--r--lib/core/libimagstore/src/store.rs12
-rw-r--r--lib/core/libimagstore/src/storeid.rs4
-rw-r--r--lib/domain/libimaghabit/src/store.rs4
-rw-r--r--lib/domain/libimagtimetrack/src/iter/get.rs8
-rw-r--r--lib/domain/libimagwiki/src/wiki.rs2
10 files changed, 181 insertions, 50 deletions
diff --git a/lib/core/libimagstore/src/file_abstraction/fs.rs b/lib/core/libimagstore/src/file_abstraction/fs.rs
index 574cace..1e47e0e 100644
--- a/lib/core/libimagstore/src/file_abstraction/fs.rs
+++ b/lib/core/libimagstore/src/file_abstraction/fs.rs
@@ -20,6 +20,7 @@
use std::fs::{File, OpenOptions, create_dir_all, remove_file, copy, rename};
use std::io::{Seek, SeekFrom, Read};
use std::path::{Path, PathBuf};
+use std::sync::Arc;
use error::{StoreError as SE, StoreErrorKind as SEK};
use error::ResultExt;
@@ -30,6 +31,9 @@ use super::Drain;
use store::Entry;
use storeid::StoreId;
use file_abstraction::iter::PathIterator;
+use file_abstraction::iter::PathIterBuilder;
+
+use walkdir::WalkDir;
#[derive(Debug)]
pub struct FSFileAbstractionInstance(PathBuf);
@@ -133,18 +137,34 @@ impl FileAbstraction for FSFileAbstraction {
})
}
- fn pathes_recursively(&self, basepath: PathBuf) -> Result<PathIterator, SE> {
- use walkdir::WalkDir;
+ fn pathes_recursively(&self,
+ basepath: PathBuf,
+ storepath: PathBuf,
+ backend: Arc<FileAbstraction>)
+ -> Result<PathIterator, SE>
+ {
+ trace!("Building PathIterator object");
+ Ok(PathIterator::new(Box::new(WalkDirPathIterBuilder { basepath }), storepath, backend))
+ }
+}
+
+pub(crate) struct WalkDirPathIterBuilder {
+ basepath: PathBuf
+}
- let i = WalkDir::new(basepath)
+impl PathIterBuilder for WalkDirPathIterBuilder {
+ fn build_iter(&self) -> Box<Iterator<Item = Result<PathBuf, SE>>> {
+ Box::new(WalkDir::new(self.basepath.clone())
.min_depth(1)
.max_open(100)
.into_iter()
.map(|r| {
r.map(|e| PathBuf::from(e.path())).chain_err(|| SE::from_kind(SEK::FileError))
- });
+ }))
+ }
- Ok(PathIterator::new(Box::new(i)))
+ fn in_collection(&mut self, c: &str) {
+ self.basepath.push(c);
}
}
diff --git a/lib/core/libimagstore/src/file_abstraction/inmemory.rs b/lib/core/libimagstore/src/file_abstraction/inmemory.rs
index 495286f..303f89d 100644
--- a/lib/core/libimagstore/src/file_abstraction/inmemory.rs
+++ b/lib/core/libimagstore/src/file_abstraction/inmemory.rs
@@ -33,6 +33,7 @@ use super::Drain;
use store::Entry;
use storeid::StoreId;
use file_abstraction::iter::PathIterator;
+use file_abstraction::iter::PathIterBuilder;
type Backend = Arc<Mutex<RefCell<HashMap<PathBuf, Entry>>>>;
@@ -181,9 +182,9 @@ impl FileAbstraction for InMemoryFileAbstraction {
Ok(())
}
- fn pathes_recursively(&self, _basepath: PathBuf) -> Result<PathIterator, SE> {
- debug!("Getting all pathes");
- let keys : Vec<Result<PathBuf, SE>> = self
+ fn pathes_recursively(&self, _basepath: PathBuf, storepath: PathBuf, backend: Arc<FileAbstraction>) -> Result<PathIterator, SE> {
+ trace!("Building PathIterator object (inmemory implementation)");
+ let keys : Vec<PathBuf> = self
.backend()
.lock()
.map_err(|_| SE::from_kind(SEK::FileError))?
@@ -191,9 +192,21 @@ impl FileAbstraction for InMemoryFileAbstraction {
.keys()
.map(PathBuf::from)
.map(Ok)
- .collect(); // we have to collect() because of the lock() above.
+ .collect::<Result<_, SE>>()?; // we have to collect() because of the lock() above.
- Ok(PathIterator::new(Box::new(keys.into_iter())))
+ Ok(PathIterator::new(Box::new(InMemPathIterBuilder(keys)), storepath, backend))
+ }
+}
+
+pub(crate) struct InMemPathIterBuilder(Vec<PathBuf>);
+
+impl PathIterBuilder for InMemPathIterBuilder {
+ fn build_iter(&self) -> Box<Iterator<Item = Result<PathBuf, SE>>> {
+ Box::new(self.0.clone().into_iter().map(Ok))
+ }
+
+ fn in_collection(&mut self, c: &str) {
+ self.0.retain(|p| p.starts_with(c));
}
}
diff --git a/lib/core/libimagstore/src/file_abstraction/iter.rs b/lib/core/libimagstore/src/file_abstraction/iter.rs
index 2cf479b..4c5dcd4 100644
--- a/lib/core/libimagstore/src/file_abstraction/iter.rs
+++ b/lib/core/libimagstore/src/file_abstraction/iter.rs
@@ -24,48 +24,63 @@ use error::Result;
use storeid::StoreId;
use file_abstraction::FileAbstraction;
+/// See documentation for PathIterator
+pub trait PathIterBuilder {
+ fn build_iter(&self) -> Box<Iterator<Item = Result<PathBuf>>>;
+ fn in_collection(&mut self, c: &str);
+}
+
/// A wrapper for an iterator over `PathBuf`s
-pub struct PathIterator(Box<Iterator<Item = Result<PathBuf>>>);
+///
+/// As the backend defines how "iterating over all entries" is implemented, this type holds a
+/// `PathIterBuilder` internally. This type is used to create new iterator instances every time the
+/// "settings" for how the iterator behaves are changed. This basically means: If the PathIterator
+/// is requested to not iterate over a directory "a" but rather its subdirectory "a/b", the
+/// implementation asks the `PathIterBuilder` to create a new iterator for that.
+///
+/// The `PathIterBuilder` can then yield a new iterator instance which is optimized for the new
+/// requirements (which basically means: Construct a new WalkDir object which does traverse the
+/// subdirectory instead of the parent).
+///
+/// This means quite a few allocations down the road, as the PathIterator itself is not generic, but
+/// this seems to be the best way to implement this.
+pub struct PathIterator {
+ iter_builder: Box<PathIterBuilder>,
+ iter: Box<Iterator<Item = Result<PathBuf>>>,
+ storepath: PathBuf,
+ backend: Arc<FileAbstraction>,
+}
impl PathIterator {
- pub fn new(iter: Box<Iterator<Item = Result<PathBuf>>>) -> PathIterator {
- PathIterator(iter)
- }
-
- pub fn store_id_constructing(self, storepath: PathBuf, backend: Arc<FileAbstraction>)
- -> StoreIdConstructingIterator
+ pub fn new(iter_builder: Box<PathIterBuilder>,
+ storepath: PathBuf,
+ backend: Arc<FileAbstraction>)
+ -> PathIterator
{
- StoreIdConstructingIterator(self, storepath, backend)
+ trace!("Generating iterator object with PathIterBuilder");
+ let iter = iter_builder.build_iter();
+ PathIterator { iter_builder, iter, storepath, backend }
}
-}
-
-impl Iterator for PathIterator {
- type Item = Result<PathBuf>;
-
- fn next(&mut self) -> Option<Self::Item> {
- self.0.next()
+ pub fn in_collection(mut self, c: &str) -> Self {
+ trace!("Generating iterator object for collection: {}", c);
+ self.iter_builder.in_collection(c);
+ self
}
}
-
-/// Helper type for constructing StoreIds from a PathIterator.
-///
-/// Automatically ignores non-files.
-pub struct StoreIdConstructingIterator(PathIterator, PathBuf, Arc<FileAbstraction>);
-
-impl Iterator for StoreIdConstructingIterator {
+impl Iterator for PathIterator {
type Item = Result<StoreId>;
fn next(&mut self) -> Option<Self::Item> {
- while let Some(next) = self.0.next() {
+ while let Some(next) = self.iter.next() {
match next {
- Err(e) => return Some(Err(e)),
- Ok(next) => match self.2.is_file(&next) {
+ Err(e) => return Some(Err(e)),
+ Ok(next) => match self.backend.is_file(&next) {
Err(e) => return Some(Err(e)),
- Ok(true) => return Some(StoreId::from_full_path(&self.1, next)),
+ Ok(true) => return Some(StoreId::from_full_path(&self.storepath, next)),
Ok(false) => { continue },
}
}
diff --git a/lib/core/libimagstore/src/file_abstraction/mod.rs b/lib/core/libimagstore/src/file_abstraction/mod.rs
index a317273..5e50db5 100644
--- a/lib/core/libimagstore/src/file_abstraction/mod.rs
+++ b/lib/core/libimagstore/src/file_abstraction/mod.rs
@@ -20,6 +20,7 @@
use std::path::PathBuf;
use std::fmt::Debug;
use std::collections::HashMap;
+use std::sync::Arc;
use error::StoreError as SE;
use store::Entry;
@@ -50,7 +51,7 @@ pub trait FileAbstraction : Debug {
fn drain(&self) -> Result<Drain, SE>;
fn fill<'a>(&'a mut self, d: Drain) -> Result<(), SE>;
- fn pathes_recursively(&self, basepath: PathBuf) -> Result<PathIterator, SE>;
+ fn pathes_recursively(&self, basepath: PathBuf, storepath: PathBuf, backend: Arc<FileAbstraction>) -> Result<PathIterator, SE>;
}
/// An abstraction trait over actions on files
diff --git a/lib/core/libimagstore/src/iter.rs b/lib/core/libimagstore/src/iter.rs
index 0e066b0..d2fd1a1 100644
--- a/lib/core/libimagstore/src/iter.rs
+++ b/lib/core/libimagstore/src/iter.rs
@@ -144,3 +144,81 @@ mod compile_test {
}
}
+use storeid::StoreId;
+use storeid::StoreIdIterator;
+use self::delete::StoreDeleteIterator;
+use self::get::StoreGetIterator;
+use self::retrieve::StoreRetrieveIterator;
+use file_abstraction::iter::PathIterator;
+use store::Store;
+use error::StoreError;
+use error::Result;
+
+/// Iterator for iterating over all (or a subset of all) entries
+///
+/// The iterator now has functionality to optimize the iteration, if only a subdirectory of the
+/// store is required, for example `$STORE/foo`.
+///
+/// This is done via functionality where the underlying iterator gets
+/// altered.
+///
+/// As the (for the filesystem backend underlying) `walkdir::WalkDir` type is not as nice as it
+/// could be, iterating over two subdirectories with one iterator is not possible. Thus, iterators
+/// for two collections in the store should be build like this (untested):
+///
+/// ```ignore
+/// store
+/// .entries()?
+/// .in_collection("foo")
+/// .chain(store.entries()?.in_collection("bar"))
+/// ```
+///
+/// Functionality to exclude subdirectories is not possible with the current implementation and has
+/// to be done during iteration, with filtering (as usual).
+pub struct Entries<'a>(PathIterator, &'a Store);
+
+impl<'a> Entries<'a> {
+
+ pub(crate) fn new(pi: PathIterator, store: &'a Store) -> Self {
+ Entries(pi, store)
+ }
+
+ pub fn in_collection(self, c: &str) -> Self {
+ Entries(self.0.in_collection(c), self.1)
+ }
+
+ pub fn without_store(self) -> StoreIdIterator {
+ StoreIdIterator::new(Box::new(self.0))
+ }
+
+ /// Transform the iterator into a StoreDeleteIterator
+ ///
+ /// This immitates the API from `libimagstore::iter`.
+ pub fn into_delete_iter(self) -> StoreDeleteIterator<'a, StoreError> {
+ StoreDeleteIterator::new(Box::new(self.0), self.1)
+ }
+
+ /// Transform the iterator into a StoreGetIterator
+ ///
+ /// This immitates the API from `libimagstore::iter`.
+ pub fn into_get_iter(self) -> StoreGetIterator<'a, StoreError> {
+ StoreGetIterator::new(Box::new(self.0), self.1)
+ }
+
+ /// Transform the iterator into a StoreRetrieveIterator
+ ///
+ /// This immitates the API from `libimagstore::iter`.
+ pub fn into_retrieve_iter(self) -> StoreRetrieveIterator<'a, StoreError> {
+ StoreRetrieveIterator::new(Box::new(self.0), self.1)
+ }
+
+}
+
+impl<'a> Iterator for Entries<'a> {
+ type Item = Result<StoreId>;
+
+ fn next(&mut self) -> Option<Self::Item> {
+ self.0.next()
+ }
+}
+
diff --git a/lib/core/libimagstore/src/store.rs b/lib/core/libimagstore/src/store.rs
index 8b19994..7923665 100644
--- a/lib/core/libimagstore/src/store.rs
+++ b/lib/core/libimagstore/src/store.rs
@@ -37,7 +37,8 @@ use toml_query::read::TomlValueReadTypeExt;
use error::{StoreError as SE, StoreErrorKind as SEK};
use error::ResultExt;
-use storeid::{IntoStoreId, StoreId, StoreIdIteratorWithStore};
+use storeid::{IntoStoreId, StoreId};
+use iter::Entries;
use file_abstraction::FileAbstractionInstance;
// We re-export the following things so tests can use them
@@ -642,12 +643,11 @@ impl Store {
}
/// Get _all_ entries in the store (by id as iterator)
- pub fn entries(&self) -> Result<StoreIdIteratorWithStore> {
+ pub fn entries<'a>(&'a self) -> Result<Entries<'a>> {
+ trace!("Building 'Entries' iterator");
self.backend
- .pathes_recursively(self.path().clone())
- .map(|i| i.store_id_constructing(self.path().clone(), self.backend.clone()))
- .map(Box::new)
- .map(|it| StoreIdIteratorWithStore::new(it, self))
+ .pathes_recursively(self.path().clone(), self.path().clone(), self.backend.clone())
+ .map(|i| Entries::new(i, self))
}
/// Gets the path where this store is on the disk
diff --git a/lib/core/libimagstore/src/storeid.rs b/lib/core/libimagstore/src/storeid.rs
index ddb1443..2f2bf72 100644
--- a/lib/core/libimagstore/src/storeid.rs
+++ b/lib/core/libimagstore/src/storeid.rs
@@ -266,6 +266,10 @@ impl StoreIdIterator {
StoreIdIterator { iter }
}
+ pub fn with_store<'a>(self, store: &'a Store) -> StoreIdIteratorWithStore<'a> {
+ StoreIdIteratorWithStore(self, store)
+ }
+
}
impl Iterator for StoreIdIterator {
diff --git a/lib/domain/libimaghabit/src/store.rs b/lib/domain/libimaghabit/src/store.rs
index f8d1a8c..b4f89bf 100644
--- a/lib/domain/libimaghabit/src/store.rs
+++ b/lib/domain/libimaghabit/src/store.rs
@@ -48,11 +48,11 @@ pub trait HabitStore {
impl HabitStore for Store {
/// Get an iterator over all habits
fn all_habit_templates(&self) -> Result<HabitTemplateStoreIdIterator> {
- self.entries().map(HabitTemplateStoreIdIterator::from).map_err(From::from)
+ Ok(HabitTemplateStoreIdIterator::from(self.entries()?.without_store()))
}
fn all_habit_instances(&self) -> Result<HabitInstanceStoreIdIterator> {
- self.entries().map(HabitInstanceStoreIdIterator::from).map_err(From::from)
+ Ok(HabitInstanceStoreIdIterator::from(self.entries()?.without_store()))
}
}
diff --git a/lib/domain/libimagtimetrack/src/iter/get.rs b/lib/domain/libimagtimetrack/src/iter/get.rs
index fdb55c2..202a87d 100644
--- a/lib/domain/libimagtimetrack/src/iter/get.rs
+++ b/lib/domain/libimagtimetrack/src/iter/get.rs
@@ -17,18 +17,18 @@
// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
//
-use libimagstore::storeid::StoreIdIteratorWithStore;
+use libimagstore::iter::Entries;
use libimagstore::store::Store;
use libimagstore::store::Result as StoreResult;
use libimagstore::store::FileLockEntry;
use constants::*;
-pub struct TimeTrackingsGetIterator<'a>(StoreIdIteratorWithStore<'a>, &'a Store);
+pub struct TimeTrackingsGetIterator<'a>(Entries<'a>, &'a Store);
impl<'a> TimeTrackingsGetIterator<'a> {
- pub fn new(sit: StoreIdIteratorWithStore<'a>, store: &'a Store) -> Self {
- TimeTrackingsGetIterator(sit, store)
+ pub fn new(entries: Entries<'a>, store: &'a Store) -> Self {
+ TimeTrackingsGetIterator(entries, store)
}
}
diff --git a/lib/domain/libimagwiki/src/wiki.rs b/lib/domain/libimagwiki/src/wiki.rs
index 0612bec..da194fd 100644
--- a/lib/domain/libimagwiki/src/wiki.rs
+++ b/lib/domain/libimagwiki/src/wiki.rs
@@ -95,7 +95,7 @@ impl<'a, 'b> Wiki<'a, 'b> {
pub fn all_ids(&self) -> Result<WikiIdIterator> {
let filter = IdIsInWikiFilter(self.1);
- Ok(WikiIdIterator(self.0.entries()?, filter))
+ Ok(WikiIdIterator(self.0.entries()?.without_store().with_store(self.0), filter))
}
pub fn delete_entry<EN: AsRef<str>>(&self, entry_name: EN) -> Result<()> {