A stack of space map tweaks.

new_block() is now a concrete method implemented using the virtual
find_free() and inc() methods.

recursive space map is better at giving correct reference counts.

persistent-data/space-maps/careful_alloc.cc

@@ -17,16 +17,12 @@
 // <http://www.gnu.org/licenses/>.
 
 #include "persistent-data/space-maps/careful_alloc.h"
-
+#include "persistent-data/space-maps/subtracting_span_iterator.h"
-#include <set>
 
 //----------------------------------------------------------------
 
 namespace {
 	class sm_careful_alloc : public checked_space_map {
-	private:
-		typedef set<block_address> block_set;
-
 	public:
 		typedef shared_ptr<sm_careful_alloc> ptr;
 
@@ -72,63 +68,9 @@ namespace {
 				mark_freed(b);
 		}
 
-		// FIXME: rewrite with tests using the run_list stuff.
+		virtual maybe_block find_free(span_iterator &it) {
-		class no_freed_blocks_iterator : public span_iterator {
+			subtracting_span_iterator filtered_it(it, freed_blocks_);
-		public:
+			return sm_->find_free(filtered_it);
-			no_freed_blocks_iterator(span_iterator &sub_it,
-						 block_set const &freed_blocks)
-				: sub_it_(sub_it),
-				  freed_blocks_(freed_blocks) {
-			}
-
-			virtual maybe_span first() {
-				current_span_ = sub_it_.first();
-
-				if (current_span_)
-					current_begin_ = current_span_->first;
-
-				return next();
-			}
-
-			virtual maybe_span next() {
-				if (!current_span_)
-					return current_span_;
-
-				if (current_begin_ == current_span_->second) {
-					current_span_ = sub_it_.next();
-					if (!current_span_)
-						return current_span_;
-
-					current_begin_ = current_span_->first;
-				}
-
-				// FIXME: slow
-				while (current_begin_ != current_span_->second &&
-				       freed_blocks_.count(current_begin_))
-					current_begin_++;
-
-				block_address b = current_begin_;
-
-				// FIXME: factor out common code
-				while (current_begin_ != current_span_->second &&
-				       !freed_blocks_.count(current_begin_))
-					current_begin_++;
-
-				block_address e = current_begin_;
-
-				return maybe_span(span(b, e));
-			}
-
-		private:
-			span_iterator &sub_it_;
-			block_set const &freed_blocks_;
-			maybe_span current_span_;
-			block_address current_begin_;
-		};
-
-		virtual maybe_block new_block(span_iterator &it) {
-			no_freed_blocks_iterator filtered_it(it, freed_blocks_);
-			return sm_->new_block(filtered_it);
 		}
 
 		virtual bool count_possibly_greater_than_one(block_address b) const {
@@ -173,7 +115,7 @@ namespace {
 		}
 
 		checked_space_map::ptr sm_;
-		block_set freed_blocks_;
+		subtracting_span_iterator::block_set freed_blocks_;
 	};
 }
 

persistent-data/space-maps/core.h

@@ -72,17 +72,14 @@ namespace persistent_data {
 				nr_free_++;
 		}
 
-		maybe_block new_block(span_iterator &it) {
+		maybe_block find_free(span_iterator &it) {
 			for (maybe_span ms = it.first(); ms; ms = it.next()) {
 				for (block_address b = ms->first; b < ms->second; b++) {
 					if (b >= counts_.size())
 						throw std::runtime_error("block out of bounds");
 
-					if (!counts_[b]) {
+					if (!counts_[b])
-						counts_[b] = 1;
-						nr_free_--;
 						return maybe_block(b);
-					}
 				}
 			}
 

persistent-data/space-maps/disk.cc

@@ -150,13 +150,9 @@ namespace {
 		}
 
 		boost::optional<unsigned> find_free(unsigned begin, unsigned end) {
-			for (unsigned i = max(begin, ie_.none_free_before_); i < end; i++) {
+			for (unsigned i = max(begin, ie_.none_free_before_); i < end; i++)
-				if (lookup(i) == 0) {
+				if (lookup(i) == 0)
-					insert(i, 1);
-					ie_.none_free_before_ = i + 1;
 					return boost::optional<unsigned>(i);
-				}
-			}
 
 			return boost::optional<unsigned>();
 		}
@@ -311,7 +307,7 @@ namespace {
 
 		// FIXME: keep track of the lowest free block so we
 		// can start searching from a suitable place.
-		maybe_block new_block(span_iterator &it) {
+		maybe_block find_free(span_iterator &it) {
 			for (maybe_span ms = it.first(); ms; ms = it.next()) {
 				block_address begin = ms->first;
 				block_address end = ms->second;
@@ -328,10 +324,7 @@ namespace {
 
 					optional<unsigned> maybe_b = bm.find_free(bit_begin, bit_end);
 					if (maybe_b) {
-						indexes_->save_ie(index, bm.get_ie());
-						nr_allocated_++;
 						block_address b = (index * ENTRIES_PER_BLOCK) + *maybe_b;
-						assert(get_count(b) == 1);
 						return b;
 					}
 				}

persistent-data/space-maps/recursive.cc

@@ -17,6 +17,7 @@
 // <http://www.gnu.org/licenses/>.
 
 #include "persistent-data/space-maps/recursive.h"
+#include "persistent-data/space-maps/subtracting_span_iterator.h"
 
 using namespace persistent_data;
 
@@ -54,7 +55,8 @@ namespace {
 	public:
 		sm_recursive(checked_space_map::ptr sm)
 			: sm_(sm),
-			  depth_(0) {
+			  depth_(0),
+			  flush_in_progress_(false) {
 		}
 
 		virtual block_address get_nr_blocks() const {
@@ -66,9 +68,29 @@ namespace {
 		}
 
 		virtual ref_t get_count(block_address b) const {
-			cant_recurse("get_count");
+			ref_t count = sm_->get_count(b);
-			recursing_const_lock lock(*this);
+
-			return sm_->get_count(b);
+			op_map::const_iterator ops_it = ops_.find(b);
+			if (ops_it != ops_.end()) {
+				list<block_op>::const_iterator it, end = ops_it->second.end();
+				for (it = ops_it->second.begin(); it != end; ++it) {
+					switch (it->op_) {
+					case block_op::INC:
+						count++;
+						break;
+
+					case block_op::DEC:
+						count--;
+						break;
+
+					case block_op::SET:
+						count = it->b_;
+						break;
+					}
+				}
+			}
+
+			return count;
 		}
 
 		virtual void set_count(block_address b, ref_t c) {
@@ -104,20 +126,44 @@ namespace {
 		}
 
 		virtual maybe_block
-		new_block(span_iterator &it) {
+		find_free(span_iterator &it) {
-			cant_recurse("new_block()");
 			recursing_lock lock(*this);
-			return sm_->new_block(it);
+
+			subtracting_span_iterator filtered_it(it, allocated_blocks_);
+			return sm_->find_free(filtered_it);
 		}
 
 		virtual bool count_possibly_greater_than_one(block_address b) const {
-			if (depth_)
+			recursing_const_lock lock(*this);
-				return true;
+			bool gto = sm_->count_possibly_greater_than_one(b);
+			if (!gto) {
+				ref_t count = 1;
 
-			else {
+				// FIXME: duplication
-				recursing_const_lock lock(*this);
+				op_map::const_iterator ops_it = ops_.find(b);
-				return sm_->count_possibly_greater_than_one(b);
+				if (ops_it != ops_.end()) {
+					list<block_op>::const_iterator it, end = ops_it->second.end();
+					for (it = ops_it->second.begin(); it != end; ++it) {
+						switch (it->op_) {
+						case block_op::INC:
+							count++;
+							break;
+
+						case block_op::DEC:
+							count--;
+							break;
+
+						case block_op::SET:
+							count = it->b_;
+							break;
+						}
+					}
+				}
+
+				gto = count > 1;
 			}
+
+			return gto;
 		}
 
 		virtual void extend(block_address extra_blocks) {
@@ -153,12 +199,23 @@ namespace {
 		}
 
 		void flush_ops() {
+			if (flush_in_progress_)
+				return;
+
+			flush_in_progress_ = true;
+			flush_ops_();
+			flush_in_progress_ = false;
+		}
+
+	private:
+		void flush_ops_() {
 			op_map::const_iterator it, end = ops_.end();
 			for (it = ops_.begin(); it != end; ++it) {
+				recursing_lock lock(*this);
+
 				list<block_op> const &ops = it->second;
 				list<block_op>::const_iterator op_it, op_end = ops.end();
 				for (op_it = ops.begin(); op_it != op_end; ++op_it) {
-					recursing_lock lock(*this);
 					switch (op_it->op_) {
 					case block_op::INC:
 						sm_->inc(op_it->b_);
@@ -176,11 +233,14 @@ namespace {
 			}
 
 			ops_.clear();
+			allocated_blocks_.clear();
 		}
 
-	private:
 		void add_op(block_op const &op) {
 			ops_[op.b_].push_back(op);
+
+			if (op.op_ == block_op::INC || (op.op_ == block_op::SET && op.rc_ > 0))
+				allocated_blocks_.insert(op.b_);
 		}
 
 		void cant_recurse(string const &method) const {
@@ -228,6 +288,9 @@ namespace {
 
 		typedef map<block_address, list<block_op> > op_map;
 		op_map ops_;
+
+		subtracting_span_iterator::block_set allocated_blocks_;
+		bool flush_in_progress_;
 	};
 }
 

persistent-data/space-maps/subtracting_span_iterator.h

@@ -71,7 +71,8 @@ namespace persistent_data {
 
 		void get_current_span_from_sub_it() {
 			current_span_ = sub_it_.next();
-			current_begin_ = current_span_->first;
+			if (current_span_)
+				current_begin_ = current_span_->first;
 		}
 
 		maybe_span next_() {

persistent-data/space_map.h

@@ -86,7 +86,7 @@ namespace persistent_data {
 			return new_block(it);
 		}
 
-		virtual maybe_block new_block(span_iterator &it) = 0;
+		virtual maybe_block find_free(span_iterator &it) = 0;
 
 		virtual bool count_possibly_greater_than_one(block_address b) const = 0;
 
@@ -101,6 +101,17 @@ namespace persistent_data {
 		virtual void iterate(iterator &it) const {
 			throw std::runtime_error("iterate() not implemented");
 		}
+
+
+		// This is a concrete method
+		maybe_block new_block(span_iterator &it) {
+			maybe_block mb = find_free(it);
+
+			if (mb)
+				inc(*mb);
+
+			return mb;
+		}
 	};
 
 	class persistent_space_map : public space_map {

persistent-data/transaction_manager.cc

@@ -50,7 +50,7 @@ transaction_manager::new_block(validator v)
 {
 	optional<block_address> mb = sm_->new_block();
 	if (!mb)
-		throw runtime_error("couldn't allocate new block");
+		throw runtime_error("transaction_manager::new_block() couldn't allocate new block");
 
 	sm_decrementer decrementer(sm_, *mb);
 	write_ref wr = bm_->write_lock_zero(*mb, v);
@@ -62,25 +62,21 @@ transaction_manager::new_block(validator v)
 pair<transaction_manager::write_ref, bool>
 transaction_manager::shadow(block_address orig, validator v)
 {
-	if (is_shadow(orig) &&
+	bool need_inc = sm_->count_possibly_greater_than_one(orig);
-	    !sm_->count_possibly_greater_than_one(orig))
+	if (is_shadow(orig) && !need_inc)
-		return make_pair(bm_->write_lock(orig, v), false);
+		return make_pair(bm_->write_lock(orig, v), need_inc);
 
 	read_ref src = bm_->read_lock(orig, v);
-
 	optional<block_address> mb = sm_->new_block();
 	if (!mb)
-		throw runtime_error("couldn't allocate new block");
+		throw runtime_error("transaction_manager::shadow() couldn't allocate new block");
 
 	write_ref dest = bm_->write_lock_zero(*mb, v);
 	::memcpy(dest.data().raw(), src.data().raw(), MD_BLOCK_SIZE); // FIXME: use buffer copy method
 
-	ref_t count = sm_->get_count(orig);
-	if (count == 0)
-		throw runtime_error("shadowing free block");
 	sm_->dec(orig);
 	add_shadow(dest.get_location());
-	return make_pair(dest, count > 1);
+	return make_pair(dest, need_inc);
 }
 
 transaction_manager::read_ref