MapBlock caching part 2

.gitignore

@@ -1,2 +1,6 @@
 /target
-**/*.rs.bk
+**/*.rs.bk
+
+# VSCode stuff
+/.vscode
+*.code_workspace

.vscode/settings.json

@@ -1,5 +0,0 @@
-{
-	"cSpell.words": [
-		"minetest"
-	]
-}

Manual.md

@@ -53,16 +53,17 @@ WorldEdit `//fixlight` command.
 
 Usage: `clone --p1 x y z --p2 x y z --offset x y z`
 
-Clone (copy) the given area to a new location.
+Clone (copy) a given area to a new location.
 
 Arguments:
 
-- `--p1, --p2`: Area to copy from.
+- `--p1, --p2`: Area to clone.
-- `--offset`: Offset to shift the area by. For example, to copy an area 50
+- `--offset`: Vector to shift the area by. For example, to copy an area 50
-nodes upward (positive Y direction), use `--offset 0 50 0`.
+nodes downward (negative Y direction), use `--offset 0 -50 0`. Directions may
+be determined using Minetest's F5 debug menu.
 
 This command copies nodes, param1, param2, and metadata. Nothing will be copied
-into mapblocks that are not yet generated.
+from or into mapblocks that are not yet generated.
 
 ### deleteblocks
 

src/commands/clone.rs

@@ -1,23 +1,42 @@
-use super::Command;
+use super::{Command, BLOCK_CACHE_SIZE};
 
-use crate::unwrap_or;
+use crate::{unwrap_or, opt_unwrap_or};
 use crate::spatial::{Vec3, area_rel_block_overlap,
 	area_abs_block_overlap};
-use crate::map_block::{MapBlock, is_valid_generated, NodeMetadataList};
+use crate::map_database::MapDatabase;
+use crate::map_block::{MapBlock, MapBlockError, is_valid_generated,
+	NodeMetadataList};
 use crate::block_utils::{merge_blocks, merge_metadata, clean_name_id_map};
 use crate::instance::{ArgType, InstBundle};
-use crate::utils::{CacheMap, CachedMapDatabase, query_keys};
+use crate::utils::{CacheMap, query_keys};
 use crate::time_keeper::TimeKeeper;
 
 
-// TODO: This and overlay--cache mapblocks in deserialized form.
+type BlockResult = Option<Result<MapBlock, MapBlockError>>;
+
+fn get_cached(
+	db: &mut MapDatabase,
+	cache: &mut CacheMap<i64, BlockResult>,
+	key: i64
+) -> BlockResult {
+	match cache.get(&key) {
+		Some(data) => data.clone(),
+		None => {
+			let block = db.get_block(key).ok()
+				.filter(|d| is_valid_generated(d))
+				.map(|d| MapBlock::deserialize(&d));
+			cache.insert(key, block.clone());
+			block
+		}
+	}
+}
 
 
 fn clone(inst: &mut InstBundle) {
 	let src_area = inst.args.area.unwrap();
 	let offset = inst.args.offset.unwrap();
 	let dst_area = src_area + offset;
-	let mut keys = query_keys(&mut inst.db, &inst.status,
+	let mut dst_keys = query_keys(&mut inst.db, &inst.status,
 		&[], Some(dst_area), false, true);
 
 	// Sort blocks according to offset such that we don't read blocks that
@@ -26,25 +45,27 @@ fn clone(inst: &mut InstBundle) {
 	// Subtract one from inverted axes to keep values from overflowing.
 	let sort_offset = sort_dir.map(|v| if v == -1 { -1 } else { 0 });
 
-	keys.sort_unstable_by_key(|k| {
+	dst_keys.sort_unstable_by_key(|k| {
 		(Vec3::from_block_key(*k) * sort_dir + sort_offset).to_block_key()
 	});
 
-	// let mut db = CachedMapDatabase::new(&mut inst.db, 256);
+	let mut block_cache = CacheMap::with_capacity(BLOCK_CACHE_SIZE);
-	let mut block_cache = CacheMap::<i64, MapBlock>::with_capacity(256);
 	let mut tk = TimeKeeper::new();
 
 	inst.status.begin_editing();
-	for dst_key in keys {
+	for dst_key in dst_keys {
 		inst.status.inc_done();
 
-		let dst_data = inst.db.get_block(dst_key).unwrap();
+		let (mut dst_block, mut dst_meta) = unwrap_or!(
-		if !is_valid_generated(&dst_data) {
+			opt_unwrap_or!(
-			continue;
+				get_cached(&mut inst.db, &mut block_cache, dst_key),
-		}
+				continue
-		let mut dst_block = MapBlock::deserialize(&dst_data).unwrap();
+			).and_then(|b|
-		let mut dst_meta = NodeMetadataList::deserialize(
+				NodeMetadataList::deserialize(b.metadata.get_ref())
-			dst_block.metadata.get_ref()).unwrap();
+					.map(|m| (b, m))
+			),
+			{ inst.status.inc_failed(); continue; }
+		);
 
 		let dst_pos = Vec3::from_block_key(dst_key);
 		let dst_part_abs = area_abs_block_overlap(&dst_area, dst_pos)
@@ -57,22 +78,15 @@ fn clone(inst: &mut InstBundle) {
 				continue;
 			}
 			let src_key = src_pos.to_block_key();
-			let src_block = if let Some(block) = block_cache.get(&src_key) {
+			let (src_block, src_meta) = opt_unwrap_or!(
-				let _t = tk.get_timer("get_block (cached)");
+				get_cached(&mut inst.db, &mut block_cache, src_key)
-				block.clone()
+					.map(Result::ok).flatten()
-			} else {
+					.and_then(|b|
-				let _t = tk.get_timer("get_block (database)");
+						NodeMetadataList::deserialize(b.metadata.get_ref())
-				let src_data = unwrap_or!(inst.db.get_block(src_key),
+							.ok().map(|m| (b, m))
-					continue);
+					),
-				if !is_valid_generated(&src_data) {
+				continue
-					continue;
+			);
-				}
-				let src_block = MapBlock::deserialize(&src_data).unwrap();
-				block_cache.insert(src_key, src_block.clone());
-				src_block
-			};
-			let src_meta = NodeMetadataList::deserialize(
-				&src_block.metadata.get_ref()).unwrap();
 
 			let src_frag_abs = area_abs_block_overlap(&src_part_abs, src_pos)
 				.unwrap();
@@ -112,8 +126,8 @@ pub fn get_command() -> Command {
 		verify_args: None,
 		args: vec![
 			(ArgType::Area(true), "Area to clone"),
-			(ArgType::Offset(true), "Vector to shift nodes by")
+			(ArgType::Offset(true), "Vector to shift the area by")
 		],
-		help: "Clone a given area to a new location."
+		help: "Clone (copy) a given area to a new location."
 	}
 }

src/commands/mod.rs

@@ -16,6 +16,9 @@ mod set_param2;
 mod vacuum;
 
 
+pub const BLOCK_CACHE_SIZE: usize = 1024;
+
+
 pub struct Command {
 	pub func: fn(&mut InstBundle),
 	pub verify_args: Option<fn(&InstArgs) -> anyhow::Result<()>>,

src/commands/overlay.rs

@@ -1,11 +1,14 @@
-use super::Command;
+use super::{Command, BLOCK_CACHE_SIZE};
 
+use crate::opt_unwrap_or;
 use crate::spatial::{Vec3, Area, area_rel_block_overlap,
 	area_abs_block_overlap, area_contains_block, area_touches_block};
 use crate::instance::{ArgType, InstArgs, InstBundle};
-use crate::map_block::{MapBlock, NodeMetadataList, is_valid_generated};
+use crate::map_database::MapDatabase;
+use crate::map_block::{MapBlock, MapBlockError, NodeMetadataList,
+	is_valid_generated};
 use crate::block_utils::{merge_blocks, merge_metadata, clean_name_id_map};
-use crate::utils::query_keys;
+use crate::utils::{query_keys, CacheMap};
 
 
 fn verify_args(args: &InstArgs) -> anyhow::Result<()> {
@@ -97,6 +100,26 @@ fn overlay_no_offset(inst: &mut InstBundle) {
 }
 
 
+type BlockResult = Option<Result<MapBlock, MapBlockError>>;
+
+fn get_cached(
+	db: &mut MapDatabase,
+	cache: &mut CacheMap<i64, BlockResult>,
+	key: i64
+) -> BlockResult {
+	match cache.get(&key) {
+		Some(data) => data.clone(),
+		None => {
+			let block = db.get_block(key).ok()
+				.filter(|d| is_valid_generated(d))
+				.map(|d| MapBlock::deserialize(&d));
+			cache.insert(key, block.clone());
+			block
+		}
+	}
+}
+
+
 /// Overlay with offset, with or without area.
 #[inline]
 fn overlay_with_offset(inst: &mut InstBundle) {
@@ -106,21 +129,27 @@ fn overlay_with_offset(inst: &mut InstBundle) {
 	let idb = inst.idb.as_mut().unwrap();
 
 	// Get keys from output database.
-	let keys = query_keys(&mut inst.db, &inst.status,
+	let dst_keys = query_keys(&mut inst.db, &inst.status,
 		&[], dst_area, inst.args.invert, true);
-	inst.status.begin_editing();
 
-	for key in keys {
+	let mut src_block_cache = CacheMap::with_capacity(BLOCK_CACHE_SIZE);
+
+	inst.status.begin_editing();
+	for dst_key in dst_keys {
 		inst.status.inc_done();
 
-		let dst_pos = Vec3::from_block_key(key);
+		let dst_pos = Vec3::from_block_key(dst_key);
-		let dst_data = inst.db.get_block(key).unwrap();
+		let dst_data = opt_unwrap_or!(
-		if !is_valid_generated(&dst_data) {
+			inst.db.get_block(dst_key).ok().filter(|d| is_valid_generated(d)),
-			continue;
+			continue
-		}
+		);
-		let mut dst_block = MapBlock::deserialize(&dst_data).unwrap();
+		let (mut dst_block, mut dst_meta) = opt_unwrap_or!(
-		let mut dst_meta = NodeMetadataList::deserialize(
+			MapBlock::deserialize(&dst_data).ok().and_then(|b|
-			dst_block.metadata.get_ref()).unwrap();
+				NodeMetadataList::deserialize(b.metadata.get_ref())
+					.ok().map(|m| (b, m))
+			),
+			{ inst.status.inc_failed(); continue; }
+		);
 
 		let dst_part_abs = dst_area.map_or(
 			Area::new(dst_pos * 16, dst_pos * 16 + 15),
@@ -133,17 +162,16 @@ fn overlay_with_offset(inst: &mut InstBundle) {
 			if !src_pos.is_valid_block_pos() {
 				continue;
 			}
-			let src_data = match idb.get_block(src_pos.to_block_key()) {
+			let src_key = src_pos.to_block_key();
-				Ok(d) => if is_valid_generated(&d) {
+			let (src_block, src_meta) = opt_unwrap_or!(
-					d
+				get_cached(idb, &mut src_block_cache, src_key)
-				} else {
+					.map(Result::ok).flatten()
-					continue
+					.and_then(|b|
-				},
+						NodeMetadataList::deserialize(b.metadata.get_ref())
-				Err(_) => continue
+							.ok().map(|m| (b, m))
-			};
+					),
-			let src_block = MapBlock::deserialize(&src_data).unwrap();
+				continue
-			let src_meta = NodeMetadataList::deserialize(
+			);
-				src_block.metadata.get_ref()).unwrap();
 
 			let src_frag_abs = area_abs_block_overlap(&src_part_abs, src_pos)
 				.unwrap();
@@ -159,7 +187,7 @@ fn overlay_with_offset(inst: &mut InstBundle) {
 
 		clean_name_id_map(&mut dst_block);
 		*dst_block.metadata.get_mut() = dst_meta.serialize(dst_block.version);
-		inst.db.set_block(key, &dst_block.serialize()).unwrap();
+		inst.db.set_block(dst_key, &dst_block.serialize()).unwrap();
 	}
 
 	inst.status.end_editing();
@@ -186,6 +214,6 @@ pub fn get_command() -> Command {
 			(ArgType::Invert, "Overlay all nodes outside the given area"),
 			(ArgType::Offset(false), "Vector to offset nodes by"),
 		],
-		help: "Copy part or all of one map into another."
+		help: "Copy part or all of one world/map into another."
 	}
 }

src/instance.rs

@@ -317,10 +317,12 @@ pub fn spawn_compute_thread(args: InstArgs)
 	-> (std::thread::JoinHandle<()>, StatusClient)
 {
 	let (status_tx, status_rx) = status_channel();
+	// Clone within this thread to avoid issue #39364 (hopefully).
+	let status_tx_2 = status_tx.clone();
 	let h = std::thread::Builder::new()
 		.name("compute".to_string())
 		.spawn(move || {
-			compute_thread(args, status_tx.clone()).unwrap_or_else(
+			compute_thread(args, status_tx_2).unwrap_or_else(
 				|err| status_tx.log_error(&err.to_string())
 			);
 		})

src/map_block/mod.rs

@@ -22,7 +22,7 @@ use node_timer::{serialize_timers, deserialize_timers};
 pub use name_id_map::NameIdMap;
 
 
-#[derive(Debug)]
+#[derive(Clone, Debug)]
 pub enum MapBlockError {
 	InvalidVersion,
 	DataError,

src/utils.rs

@@ -5,8 +5,7 @@ use memmem::{Searcher, TwoWaySearcher};
 use byteorder::{WriteBytesExt, BigEndian};
 
 use crate::instance::{InstState, StatusServer};
-use crate::map_block::MapBlock;
+use crate::map_database::MapDatabase;
-use crate::map_database::{MapDatabase, DBError};
 use crate::spatial::{Area, Vec3};
 
 
@@ -101,36 +100,6 @@ impl<K: Eq + std::hash::Hash + Clone, V> CacheMap<K, V> {
 }
 
 
-pub struct CachedMapDatabase<'a, 'b> {
-	db: &'a mut MapDatabase<'b>,
-	cache: CacheMap<i64, Option<MapBlock>>
-}
-
-impl<'a, 'b> CachedMapDatabase<'a, 'b> {
-	pub fn new(db: &'a mut MapDatabase<'b>, cap: usize) -> Self {
-		Self { db, cache: CacheMap::with_capacity(cap) }
-	}
-
-	pub fn get_block(&mut self, key: i64) -> Option<MapBlock> {
-		if let Some(block) = self.cache.get(&key) {
-			block.clone()
-		} else {
-			let data = self.db.get_block(key).ok();
-			let block = match data {
-				Some(d) => MapBlock::deserialize(&d).ok(),
-				None => None
-			};
-			self.cache.insert(key, block.clone());
-			block
-		}
-	}
-
-	pub fn set_block(&mut self, key: i64, data: &[u8]) -> Result<(), DBError> {
-		self.db.set_block(key, data)
-	}
-}
-
-
 pub fn to_bytes(s: &String) -> Vec<u8> {
 	s.as_bytes().to_vec()
 }
@@ -155,6 +124,17 @@ macro_rules! unwrap_or {
 }
 
 
+#[macro_export]
+macro_rules! opt_unwrap_or {
+	($res:expr, $alt:expr) => {
+		match $res {
+			Some(val) => val,
+			None => $alt
+		}
+	}
+}
+
+
 pub fn fmt_duration(dur: Duration) -> String {
 	let s = dur.as_secs();
 	if s < 3600 {

workspace.code-workspace

@@ -1,7 +0,0 @@
-{
-	"folders": [
-		{
-			"path": "."
-		}
-	]
-}