Cleanup sqlite3 database code a bit

As tests using --sqlite-cacheworldrow showed a consistently
lower performance than without, this option was effectively
disabled. It is still recognised for compatibility, but
it may be removed some time in the future.

TileGenerator.cpp

@@ -150,7 +150,6 @@ TileGenerator::TileGenerator():
 	m_shrinkGeometry(true),
 	m_blockGeometry(false),
 	m_scaleFactor(1),
-	m_sqliteCacheWorldRow(false),
 	m_chunkSize(0),
 	m_sideScaleMajor(0),
 	m_sideScaleMinor(0),
@@ -232,11 +231,6 @@ void TileGenerator::setBlockGeometry(bool block)
 	m_blockGeometry = block;
 }
 
-void TileGenerator::setSqliteCacheWorldRow(bool cacheWorldRow)
-{
-	m_sqliteCacheWorldRow = cacheWorldRow;
-}
-
 void TileGenerator::setScaleColor(const Color &scaleColor)
 {
 	m_scaleColor = scaleColor;
@@ -792,7 +786,6 @@ void TileGenerator::openDb(const std::string &input)
 #if USE_SQLITE3
 		DBSQLite3 *db;
 		m_db = db = new DBSQLite3(input);
-		db->cacheWorldRow = m_sqliteCacheWorldRow;
 #else
 		unsupported = true;
 #endif
@@ -1560,9 +1553,8 @@ void TileGenerator::renderMap()
 	}
 	if (verboseStatistics) {
 		cout << "Statistics"
-		     << ":  blocks read: " << m_db->getBlocksReadCount()
+		     << ":  blocks read/queried: " << m_db->getBlocksReadCount()
-		     << "  (" << m_db->getBlocksCachedCount() << " cached + "
+		     << " / " << m_db->getBlocksQueriedCount()
-			       << m_db->getBlocksUnCachedCount() << " uncached)"
 		     << ";  blocks rendered: " << blocks_rendered
 		     << ";  area rendered: " << area_rendered
 		     << "/" << (m_xMax-m_xMin+1) * (m_zMax-m_zMin+1)

TileGenerator.h

@@ -134,7 +134,6 @@ public:
 	void setMaxY(int y);
 	void setShrinkGeometry(bool shrink);
 	void setBlockGeometry(bool block);
-	void setSqliteCacheWorldRow(bool cacheWorldRow);
 	void setTileBorderColor(const Color &tileBorderColor);
 	void setTileBorderSize(int size);
 	void setTileSize(int width, int heigth);
@@ -235,7 +234,6 @@ private:
 	bool m_shrinkGeometry;
 	bool m_blockGeometry;
 	int m_scaleFactor;
-	bool m_sqliteCacheWorldRow;
 	int m_chunkSize;
 	int m_sideScaleMajor;
 	int m_sideScaleMinor;

db-leveldb.cpp

@@ -18,7 +18,7 @@ inline std::string i64tos(int64_t i) {
 
 DBLevelDB::DBLevelDB(const std::string &mapdir) :
 	m_blocksReadCount(0),
-	m_blocksUnCachedCount(0)
+	m_blocksQueriedCount(0)
 {
 	leveldb::Options options;
 	options.create_if_missing = false;
@@ -36,14 +36,9 @@ int DBLevelDB::getBlocksReadCount(void)
 	return m_blocksReadCount;
 }
 
-int DBLevelDB::getBlocksCachedCount(void)
+int DBLevelDB::getBlocksQueriedCount(void)
 {
-	return 0;
+	return m_blocksQueriedCount;
-}
-
-int DBLevelDB::getBlocksUnCachedCount(void)
-{
-	return m_blocksUnCachedCount;
 }
 
 const DB::BlockPosList &DBLevelDB::getBlockPos() {
@@ -61,11 +56,11 @@ DB::Block DBLevelDB::getBlockOnPos(const BlockPos &pos)
 	std::string datastr;
 	leveldb::Status status;
 
-	m_blocksReadCount++;
+	m_blocksQueriedCount++;
 
 	status = m_db->Get(leveldb::ReadOptions(), pos.databasePosStr(), &datastr);
 	if(status.ok()) {
-		m_blocksUnCachedCount++;
+		m_blocksReadCount++;
 		return Block(pos, ustring(reinterpret_cast<const unsigned char *>(datastr.c_str()), datastr.size()));
 	}
 	else {

db-leveldb.h

@@ -8,15 +8,14 @@
 class DBLevelDB : public DB {
 public:
 	DBLevelDB(const std::string &mapdir);
-	virtual int getBlocksUnCachedCount(void);
+	virtual int getBlocksQueriedCount(void);
-	virtual int getBlocksCachedCount(void);
 	virtual int getBlocksReadCount(void);
 	virtual const BlockPosList &getBlockPos();
 	virtual Block getBlockOnPos(const BlockPos &pos);
 	~DBLevelDB();
 private:
 	int m_blocksReadCount;
-	int m_blocksUnCachedCount;
+	int m_blocksQueriedCount;
 	leveldb::DB *m_db;
 	BlockPosList m_blockPosList;
 };

db-redis.cpp

@@ -86,7 +86,7 @@ std::string get_setting_default(std::string name, std::istream &is, const std::s
 
 DBRedis::DBRedis(const std::string &mapdir) :
 	m_blocksReadCount(0),
-	m_blocksUnCachedCount(0)
+	m_blocksQueriedCount(0)
 {
 	std::ifstream ifs((mapdir + "/world.mt").c_str());
 	if(!ifs.good())
@@ -124,16 +124,9 @@ int DBRedis::getBlocksReadCount(void)
 	return m_blocksReadCount;
 }
 
-
+int DBRedis::getBlocksQueriedCount(void)
-int DBRedis::getBlocksCachedCount(void)
 {
-	return 0;
+	return m_blocksQueriedCount;
-}
- 
-
-int DBRedis::getBlocksUnCachedCount(void)
-{
-	return m_blocksUnCachedCount;
 }
 
 
@@ -162,13 +155,13 @@ DB::Block DBRedis::getBlockOnPos(const BlockPos &pos)
 	std::string tmp;
 	Block block(pos,reinterpret_cast<const unsigned char *>(""));
 
-	m_blocksReadCount++;
+	m_blocksQueriedCount++;
 
 	reply = (redisReply*) redisCommand(ctx, "HGET %s %s", hash.c_str(), pos.databasePosStr().c_str());
 	if(!reply)
 		throw std::runtime_error(std::string("redis command 'HGET %s %s' failed: ") + ctx->errstr);
 	if (reply->type == REDIS_REPLY_STRING && reply->len != 0) {
-		m_blocksUnCachedCount++;
+		m_blocksReadCount++;
 		block = Block(pos, ustring(reinterpret_cast<const unsigned char *>(reply->str), reply->len));
 	} else
 		throw std::runtime_error("Got wrong response to 'HGET %s %s' command");

db-redis.h

@@ -7,15 +7,14 @@
 class DBRedis : public DB {
 public:
 	DBRedis(const std::string &mapdir);
-	virtual int getBlocksUnCachedCount(void);
+	virtual int getBlocksQueriedCount(void);
-	virtual int getBlocksCachedCount(void);
 	virtual int getBlocksReadCount(void);
 	virtual const BlockPosList &getBlockPos();
 	virtual Block getBlockOnPos(const BlockPos &pos);
 	~DBRedis();
 private:
 	int m_blocksReadCount;
-	int m_blocksUnCachedCount;
+	int m_blocksQueriedCount;
 	redisContext *ctx;
 	std::string hash;
 	BlockPosList m_blockPosList;

db-sqlite3.cpp

@@ -3,14 +3,14 @@
 #include <unistd.h> // for usleep
 #include "types.h"
 
+#define BLOCKPOSLIST_STATEMENT	"SELECT pos FROM blocks"
+#define BLOCK_STATEMENT		"SELECT pos, data FROM blocks WHERE pos == ?"
+
 
 DBSQLite3::DBSQLite3(const std::string &mapdir) :
-	cacheWorldRow(false),
+	m_blocksQueriedCount(0),
 	m_blocksReadCount(0),
-	m_blocksCachedCount(0),
-	m_blocksUnCachedCount(0),
 	m_blockPosListStatement(NULL),
-	m_blocksOnZStatement(NULL),
 	m_blockOnPosStatement(NULL)
 {
 	
@@ -18,11 +18,16 @@ DBSQLite3::DBSQLite3(const std::string &mapdir) :
 	if (sqlite3_open_v2(db_name.c_str(), &m_db, SQLITE_OPEN_READONLY | SQLITE_OPEN_PRIVATECACHE, 0) != SQLITE_OK) {
 		throw std::runtime_error(std::string(sqlite3_errmsg(m_db)) + ", Database file: " + db_name);
 	}
+	if (SQLITE_OK != sqlite3_prepare_v2(m_db, BLOCKPOSLIST_STATEMENT, sizeof(BLOCKPOSLIST_STATEMENT)-1, &m_blockPosListStatement, 0)) {
+		throw std::runtime_error("Failed to prepare SQL statement (blockPosListStatement)");
+	}
+	if (SQLITE_OK != sqlite3_prepare_v2(m_db, BLOCK_STATEMENT, sizeof(BLOCK_STATEMENT)-1, &m_blockOnPosStatement, 0)) {
+		throw std::runtime_error("Failed to prepare SQL statement (blockOnPosStatement)");
+	}
 }
 
 DBSQLite3::~DBSQLite3() {
 	if (m_blockPosListStatement) sqlite3_finalize(m_blockPosListStatement);
-	if (m_blocksOnZStatement) sqlite3_finalize(m_blocksOnZStatement);
 	if (m_blockOnPosStatement) sqlite3_finalize(m_blockOnPosStatement);
 	sqlite3_close(m_db);
 }
@@ -32,78 +37,34 @@ int DBSQLite3::getBlocksReadCount(void)
 	return m_blocksReadCount;
 }
 
-int DBSQLite3::getBlocksCachedCount(void)
+int DBSQLite3::getBlocksQueriedCount(void)
 {
-	return m_blocksCachedCount;
+	return m_blocksQueriedCount;
-}
-
-int DBSQLite3::getBlocksUnCachedCount(void)
-{
-	return m_blocksUnCachedCount;
 }
 
 const DB::BlockPosList &DBSQLite3::getBlockPos() {
 	m_BlockPosList.clear();
-	std::string sql = "SELECT pos FROM blocks";
+	int result = 0;
-	if (m_blockPosListStatement || sqlite3_prepare_v2(m_db, sql.c_str(), sql.length(), &m_blockPosListStatement, 0) == SQLITE_OK) {
+	while (true) {
-		int result = 0;
+		result = sqlite3_step(m_blockPosListStatement);
-		while (true) {
+		if(result == SQLITE_ROW) {
-			result = sqlite3_step(m_blockPosListStatement);
+			sqlite3_int64 blocknum = sqlite3_column_int64(m_blockPosListStatement, 0);
-			if(result == SQLITE_ROW) {
+			m_BlockPosList.push_back(blocknum);
-				sqlite3_int64 blocknum = sqlite3_column_int64(m_blockPosListStatement, 0);
+		} else if (result == SQLITE_BUSY) // Wait some time and try again
-				m_BlockPosList.push_back(blocknum);
+			usleep(10000);
-			} else if (result == SQLITE_BUSY) // Wait some time and try again
+		else
-				usleep(10000);
+			break;
-			else
-				break;
-		}
-	} else {
-		sqlite3_reset(m_blockPosListStatement);
-		throw std::runtime_error("Failed to get list of MapBlocks");
 	}
 	sqlite3_reset(m_blockPosListStatement);
 	return m_BlockPosList;
 }
 
-void DBSQLite3::prepareBlocksOnZStatement(void)
+
+DB::Block DBSQLite3::getBlockOnPos(const BlockPos &pos)
 {
-	//std::string sql = "SELECT pos, data FROM blocks WHERE (pos >= ? AND pos <= ?)";
-	std::string sql = "SELECT pos, data FROM blocks WHERE (pos BETWEEN ? AND ?)";
-	if (!m_blocksOnZStatement && sqlite3_prepare_v2(m_db, sql.c_str(), sql.length(), &m_blocksOnZStatement, 0) != SQLITE_OK) {
-		throw std::runtime_error("Failed to prepare statement (blocksOnZStatement)");
-	}
-}
-
-void DBSQLite3::prepareBlockOnPosStatement(void)
-{
-	std::string sql = "SELECT pos, data FROM blocks WHERE pos == ?";
-	if (!m_blockOnPosStatement && sqlite3_prepare_v2(m_db, sql.c_str(), sql.length(), &m_blockOnPosStatement, 0) != SQLITE_OK) {
-		throw std::runtime_error("Failed to prepare SQL statement (blockOnPosStatement)");
-	}
-}
-
-void DBSQLite3::cacheBlocksOnZRaw(int zPos)
-{
-	prepareBlocksOnZStatement();
-
-	sqlite3_int64 psMin;
-	sqlite3_int64 psMax;
-
-	psMin = (static_cast<sqlite3_int64>(zPos) * 0x1000000L) - 0x800000;
-	psMax = (static_cast<sqlite3_int64>(zPos) * 0x1000000L) + 0x7fffff;
-
-	sqlite3_bind_int64(m_blocksOnZStatement, 1, psMin);
-	sqlite3_bind_int64(m_blocksOnZStatement, 2, psMax);
-
-	cacheBlocks(m_blocksOnZStatement);
-}
-
-DB::Block DBSQLite3::getBlockOnPosRaw(const BlockPos &pos)
-{
-	prepareBlockOnPosStatement();
-
 	Block block(pos,reinterpret_cast<const unsigned char *>(""));
 	int result = 0;
+	m_blocksQueriedCount++;
 
 	sqlite3_bind_int64(m_blockOnPosStatement, 1, pos.databasePosI64());
 
@@ -113,7 +74,7 @@ DB::Block DBSQLite3::getBlockOnPosRaw(const BlockPos &pos)
 			const unsigned char *data = reinterpret_cast<const unsigned char *>(sqlite3_column_blob(m_blockOnPosStatement, 1));
 			int size = sqlite3_column_bytes(m_blockOnPosStatement, 1);
 			block = Block(pos, ustring(data, size));
-			m_blocksUnCachedCount++;
+			m_blocksReadCount++;
 			break;
 		} else if (result == SQLITE_BUSY) { // Wait some time and try again
 			usleep(10000);
@@ -126,50 +87,3 @@ DB::Block DBSQLite3::getBlockOnPosRaw(const BlockPos &pos)
 	return block;
 }
 
-void DBSQLite3::cacheBlocks(sqlite3_stmt *SQLstatement)
-{
-	int result = 0;
-	while (true) {
-		result = sqlite3_step(SQLstatement);
-		if(result == SQLITE_ROW) {
-			sqlite3_int64 blocknum = sqlite3_column_int64(SQLstatement, 0);
-			const unsigned char *data = reinterpret_cast<const unsigned char *>(sqlite3_column_blob(SQLstatement, 1));
-			int size = sqlite3_column_bytes(SQLstatement, 1);
-			m_blockCache[blocknum] = ustring(data, size);
-			m_blocksCachedCount++;
-		} else if (result == SQLITE_BUSY) { // Wait some time and try again
-			usleep(10000);
-		} else {
-			break;
-		}
-	}
-	sqlite3_reset(SQLstatement);
-}
-
-DB::Block DBSQLite3::getBlockOnPos(const BlockPos &pos)
-{
-	m_blocksReadCount++;
-
-	BlockCache::const_iterator DBBlockSearch;
-	DBBlockSearch = m_blockCache.find(pos.databasePosI64());
-	if (DBBlockSearch == m_blockCache.end()) {
-		if (cacheWorldRow) {
-			m_blockCache.clear();
-			cacheBlocksOnZRaw(pos.z);
-			DBBlockSearch = m_blockCache.find(pos.databasePosI64());
-			if (DBBlockSearch != m_blockCache.end()) {
-				return Block(pos, DBBlockSearch->second);
-			}
-			else {
-				return Block(pos,reinterpret_cast<const unsigned char *>(""));
-			}
-		}
-		else {
-			return getBlockOnPosRaw(pos);
-		}
-	}
-	else {
-		return Block(pos, DBBlockSearch->second);
-	}
-}
-

db-sqlite3.h

@@ -20,29 +20,23 @@ class DBSQLite3 : public DB {
 	typedef std::map<int64_t, ustring>  BlockCache;
 #endif
 public:
-	bool cacheWorldRow;
 	DBSQLite3(const std::string &mapdir);
-	virtual int getBlocksUnCachedCount(void);
+	virtual int getBlocksQueriedCount(void);
-	virtual int getBlocksCachedCount(void);
 	virtual int getBlocksReadCount(void);
 	virtual const BlockPosList &getBlockPos();
 	virtual Block getBlockOnPos(const BlockPos &pos);
 	~DBSQLite3();
 private:
+	int m_blocksQueriedCount;
 	int m_blocksReadCount;
-	int m_blocksCachedCount;
-	int m_blocksUnCachedCount;
 	sqlite3 *m_db;
 	sqlite3_stmt *m_blockPosListStatement;
-	sqlite3_stmt *m_blocksOnZStatement;
 	sqlite3_stmt *m_blockOnPosStatement;
 	std::ostringstream  m_getBlockSetStatementBlocks;
 	BlockCache  m_blockCache;
 	BlockPosList m_BlockPosList;
 
-	void prepareBlocksOnZStatement(void);
 	void prepareBlockOnPosStatement(void);
-	void cacheBlocksOnZRaw(int zPos);
 	Block getBlockOnPosRaw(const BlockPos &pos);
 	void cacheBlocks(sqlite3_stmt *SQLstatement);
 };

db.h

@@ -15,8 +15,7 @@ public:
 	typedef std::pair<BlockPos, ustring> Block;
 	typedef std::vector<BlockPos>  BlockPosList;
 	virtual const BlockPosList &getBlockPos()=0;
-	virtual int getBlocksUnCachedCount(void)=0;
+	virtual int getBlocksQueriedCount(void)=0;
-	virtual int getBlocksCachedCount(void)=0;
 	virtual int getBlocksReadCount(void)=0;
 	virtual Block getBlockOnPos(const BlockPos &pos)=0;
 };

doc/manual.rst

@@ -163,7 +163,6 @@ Miscellaneous options
 .....................
 
     * ``--backend <auto/sqlite3/leveldb/redis>`` :	Specify or override the database backend to use
-    * ``--sqlite-cacheworldrow`` :			Modify how minetestmapper accesses the sqlite3 database. For performance.
 
 
 Detailed Description of Options
@@ -839,15 +838,9 @@ Detailed Description of Options
 
 ``--sqlite-cacheworldrow``
 ..........................
-	Modify the way minetestmapper accesses the sqlite3 database.
+	This option is no longer supported, as minetestmapper performed
-
+	consistently worse with it than without it, as tested on a few
-	When using sqlite3, read an entire world row at one, instead of reading
+	large worlds.
-	one block at a time.
-
-	This option was added to possibly achieve better performance
-	in some cases where a complete map is drawn of a very large world.
-
-	It may or may not have the desired effect. Any feedback is welcome.
 
 ``--tilebordercolor <color>``
 .............................

mapper.cpp

@@ -114,9 +114,6 @@ void usage()
 			"\tblock:   round geometry away from zero, to entire map blocks (16 nodes)\n"
 			"\tfixed:   generate a map of exactly the requested geometry\n"
 			"\tshrink:  generate a smaller map if possible\n"
-#if USE_SQLITE3
-			"  --sqlite-cacheworldrow\n"
-#endif
 			"  --tiles <tilesize>[+<border>]|block|chunk\n"
 			"  --tileorigin <x>,<y>|world|map\n"
 			"  --tilecenter <x>,<y>|world|map\n"
@@ -840,7 +837,11 @@ int main(int argc, char *argv[])
 					generator.setShading(false);
 					break;
 				case OPT_SQLITE_CACHEWORLDROW:
-					generator.setSqliteCacheWorldRow(true);
+					// This option is recognised for backward compatibility.
+					// Tests with a (large) world on SSD and on HDD showed a performance decrease
+					// on all map sizes with this option enabled.
+					// (Next: print a message when this option is used.
+					//  Later: remove it completely)
 					break;
 				case OPT_PROGRESS_INDICATOR:
 					generator.enableProgressIndicator();