widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

SirVer has proposed merging lp:~widelands-dev/widelands/resize_texture_cache into lp:widelands.

Commit message:
- Resize texture cache to 30 MB.
- Simplify texture cache: It only deals with transient surfaces and uses less virtual functions and no pointers.

Fixes bug 1121944.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/resize_texture_cache/+merge/283048

Simplifies the texture cache used by the new font renderer. 


-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/resize_texture_cache into lp:widelands.

=== modified file 'src/graphic/font_handler1.cc'
--- src/graphic/font_handler1.cc	2016-01-09 15:27:05 +0000
+++ src/graphic/font_handler1.cc	2016-01-19 08:57:55 +0000
@@ -45,9 +45,14 @@
 
 namespace {
 
-/// The size of the richtext surface cache in bytes. All work that the richtext
-/// renderer does is / cached in this cache until it overflows.
-const uint32_t RICHTEXT_SURFACE_CACHE = 160 << 20;   // shifting converts to MB
+// The size of the richtext surface cache in bytes. All work that the richtext
+// renderer does is / cached in this cache until it overflows. The idea is that
+// this is big enough to cache the text that is used on a typical screen - so
+// that we do not need to lay out text every frame. Last benchmarked at r7712,
+// 30 MB was enough to cache texts for many frames (> 1000), while it is
+// quickly overflowing in the map selection menu.
+// This might need reevaluation is the new font handler is used for more stuff.
+const uint32_t RICHTEXT_TEXTURE_CACHE = 30 << 20;   // shifting converts to MB
 
 // An Image implementation that recreates a rich text texture when needed on
 // the fly. It is meant to be saved into the ImageCache.
@@ -80,9 +85,8 @@
 		if (surf)
 			return surf;
 
-		surf = get_renderer_()->render(text_, width_);
-		texture_cache_->insert(hash_, surf, true);
-		return surf;
+		return texture_cache_->insert(
+		   hash_, std::unique_ptr<Texture>(get_renderer_()->render(text_, width_)));
 	}
 
 	const string hash_;
@@ -104,7 +108,7 @@
 class FontHandler1 : public IFontHandler1 {
 public:
 	FontHandler1(ImageCache* image_cache)
-	   : texture_cache_(create_texture_cache(RICHTEXT_SURFACE_CACHE)),
+	   : texture_cache_(new TextureCache(RICHTEXT_TEXTURE_CACHE)),
 	     fontset_(new UI::FontSet(i18n::get_locale())),
 	     rt_renderer_(new RT::Renderer(image_cache, texture_cache_.get(), fontset_.get())),
 	     image_cache_(image_cache) {

=== modified file 'src/graphic/text/sdl_ttf_font.cc'
--- src/graphic/text/sdl_ttf_font.cc	2014-11-24 07:10:03 +0000
+++ src/graphic/text/sdl_ttf_font.cc	2016-01-19 08:57:55 +0000
@@ -120,7 +120,7 @@
 	if (!text_surface)
 		throw RenderError((format("Rendering '%s' gave the error: %s") % txt % TTF_GetError()).str());
 
-	return *texture_cache->insert(hash, new Texture(text_surface), true);
+	return *texture_cache->insert(hash, std::unique_ptr<Texture>(new Texture(text_surface)));
 }
 
 uint16_t SdlTtfFont::ascent(int style) const {

=== modified file 'src/graphic/text/test/render.cc'
--- src/graphic/text/test/render.cc	2014-12-14 12:16:27 +0000
+++ src/graphic/text/test/render.cc	2016-01-19 08:57:55 +0000
@@ -35,7 +35,7 @@
 	g_fs->add_file_system(&FileSystem::create(WIDELANDS_DATA_DIR));
 	g_fs->add_file_system(&FileSystem::create(RICHTEXT_DATA_DIR));
 
-	texture_cache_.reset(create_texture_cache(500 << 20));  // 500 MB
+	texture_cache_.reset(new TextureCache(500 << 20));  // 500 MB
 	image_cache_.reset(new ImageCache());
 	renderer_.reset(new RT::Renderer(image_cache_.get(), texture_cache_.get(), new UI::FontSet("en")));
 }

=== modified file 'src/graphic/texture_cache.cc'
--- src/graphic/texture_cache.cc	2014-11-24 07:10:03 +0000
+++ src/graphic/texture_cache.cc	2016-01-19 08:57:55 +0000
@@ -19,117 +19,68 @@
 
 #include "graphic/texture_cache.h"
 
-#include <cassert>
-#include <list>
-#include <map>
-#include <memory>
+#include <stdint.h>
 
 #include <SDL.h>
 
 #include "graphic/texture.h"
-
-using namespace std;
-
-// I took inspiration from http://timday.bitbucket.org/lru.html, but our use
-// case here is a little different.
-namespace  {
-class TextureCacheImpl : public TextureCache {
-public:
-	TextureCacheImpl(uint32_t max_transient_memory) :
-		max_transient_memory_(max_transient_memory), used_transient_memory_(0) {}
-	virtual ~TextureCacheImpl();
-
-	// Implements TextureCache.
-	void flush() override;
-	Texture* get(const string& hash) override;
-	Texture* insert(const string& hash, Texture*, bool) override;
-
-private:
-	void drop();
-
-	using AccessHistory = list<string>;
-	struct Entry {
-		Entry(Texture* gs, const AccessHistory::iterator& it, bool transient) :
-			texture(gs), is_transient(transient), last_access(SDL_GetTicks()), list_iterator(it) {}
-
-		std::unique_ptr<Texture> texture;
-		bool is_transient;
-		uint32_t last_access;  // Mainly for debugging and analysis.
-		const AccessHistory::iterator list_iterator;  // Only valid if is_transient is true.
-	};
-	using Container = map<string, Entry*>;
-
-	uint32_t max_transient_memory_;
-	uint32_t used_transient_memory_;
-	Container entries_;
-	AccessHistory access_history_;
-};
-
-TextureCacheImpl::~TextureCacheImpl() {
+#include "base/log.h" // NOCOM(#sirver): remove again
+
+// The implementation took inspiration from
+// http://timday.bitbucket.org/lru.html, but our use case here is a little
+// different.
+
+TextureCache::TextureCache(uint32_t max_size_in_bytes) :
+	max_size_in_bytes_(max_size_in_bytes), size_in_bytes_(0) {}
+
+TextureCache::~TextureCache() {
 	flush();
 }
 
-void TextureCacheImpl::flush() {
-	for (Container::iterator it = entries_.begin(); it != entries_.end(); ++it) {
-		delete it->second;
-	}
+void TextureCache::flush() {
 	entries_.clear();
 	access_history_.clear();
-	used_transient_memory_ = 0;
+	size_in_bytes_ = 0;
 }
 
-Texture* TextureCacheImpl::get(const string& hash) {
-	const Container::iterator it = entries_.find(hash);
+Texture* TextureCache::get(const std::string& hash) {
+	const auto it = entries_.find(hash);
 	if (it == entries_.end())
 		return nullptr;
 
 	// Move this to the back of the access list to signal that we have used this
 	// recently and update last access time.
-	if (it->second->is_transient) {
-		access_history_.splice(access_history_.end(), access_history_, it->second->list_iterator);
-		it->second->last_access = SDL_GetTicks();
-	}
-	return it->second->texture.get();
+	access_history_.splice(access_history_.end(), access_history_, it->second.list_iterator);
+	it->second.last_access = SDL_GetTicks();
+	return it->second.texture.get();
 }
 
-Texture* TextureCacheImpl::insert(const string& hash, Texture* texture, bool transient) {
+Texture* TextureCache::insert(const std::string& hash, std::unique_ptr<Texture> texture) {
 	assert(entries_.find(hash) == entries_.end());
 
-	if (transient) {
-		const uint32_t texture_size = texture->width() * texture->height() * 4;
-		while (used_transient_memory_ + texture_size > max_transient_memory_) {
-			drop();
-		}
-
-		// Record hash as most-recently-used.
-		AccessHistory::iterator it = access_history_.insert(access_history_.end(), hash);
-		used_transient_memory_ += texture_size;
-		entries_.insert(make_pair(hash, new Entry(texture, it, true)));
-	} else {
-		entries_.insert(make_pair(hash, new Entry(texture, access_history_.end(), false)));
+	const uint32_t texture_size = texture->width() * texture->height() * 4;
+	while (size_in_bytes_ + texture_size > max_size_in_bytes_) {
+		drop();
 	}
-	return texture;
+
+	// Record hash as most-recently-used.
+	AccessHistory::iterator it = access_history_.insert(access_history_.end(), hash);
+	size_in_bytes_ += texture_size;
+	return entries_.insert(make_pair(hash, Entry{std::move(texture), SDL_GetTicks(), it}))
+	   .first->second.texture.get();
 }
 
-void TextureCacheImpl::drop() {
+void TextureCache::drop() {
 	assert(!access_history_.empty());
 
 	// Identify least recently used key
-	const Container::iterator it = entries_.find(access_history_.front());
+	const auto it = entries_.find(access_history_.front());
 	assert(it != entries_.end());
-	assert(it->second->is_transient);
 
-	const uint32_t texture_size = it->second->texture->width() * it->second->texture->height() * 4;
-	used_transient_memory_ -= texture_size;
+	const uint32_t texture_size = it->second.texture->width() * it->second.texture->height() * 4;
+	size_in_bytes_ -= texture_size;
 
 	// Erase both elements to completely purge record
-	delete it->second;
 	entries_.erase(it);
 	access_history_.pop_front();
 }
-
-}  // namespace
-
-TextureCache* create_texture_cache(uint32_t transient_memory_in_bytes) {
-	return new TextureCacheImpl(transient_memory_in_bytes);
-}

=== modified file 'src/graphic/texture_cache.h'
--- src/graphic/texture_cache.h	2014-11-24 07:10:03 +0000
+++ src/graphic/texture_cache.h	2016-01-19 08:57:55 +0000
@@ -20,6 +20,10 @@
 #ifndef WL_GRAPHIC_TEXTURE_CACHE_H
 #define WL_GRAPHIC_TEXTURE_CACHE_H
 
+#include <cassert>
+#include <list>
+#include <map>
+#include <memory>
 #include <string>
 
 #include <boost/utility.hpp>
@@ -28,39 +32,49 @@
 
 class Texture;
 
-// Caches Surfaces. It contains surfaces which must not be deleted and
-// transient surfaces that are always free to be deleted - somebody else must
-// then recreate them when they are needed again.
+// Caches transient Surfaces, i.e. those that are always free to be deleted
+// because they can be regenerated - somebody else must then recreate them when
+// they are needed again.
 //
-// Nobody in Widelands should hold onto a Surface they get from this class,
+// Nothing in Widelands should hold onto a Surface they get from this class,
 // instead, they should use it only temporarily and rerequest it whenever they
 // need it.
 class TextureCache {
 public:
-	TextureCache() {}
-	virtual ~TextureCache() {}
+	// Create a new Cache whichs combined pixels data in all transient surfaces
+	// are always below the 'max_size_in_bytes'.
+	TextureCache(uint32_t max_size_in_bytes);
+	~TextureCache();
 
 	/// Deletes all surfaces in the cache leaving it as if it were just created.
-	virtual void flush() = 0;
+	void flush();
 
 	/// Returns an entry if it is cached, nullptr otherwise.
-	virtual Texture* get(const std::string& hash) = 0;
+	Texture* get(const std::string& hash);
 
 	// Inserts this entry into the TextureCache. asserts() that there is no
 	// entry with this hash already cached. Returns the given Surface for
 	// convenience. If 'transient' is false, this surface will not be deleted
 	// automatically - use this if surfaces are around for a long time and
 	// recreation is expensive (i.e. images loaded from disk).
-	virtual Texture* insert(const std::string& hash, Texture*, bool transient) = 0;
+	Texture* insert(const std::string& hash, std::unique_ptr<Texture> texture);
 
 private:
+	void drop();
+
+	using AccessHistory = std::list<std::string>;
+	struct Entry {
+		std::unique_ptr<Texture> texture;
+		uint32_t last_access;  // Mainly for debugging and analysis.
+		const AccessHistory::iterator list_iterator;
+	};
+
+	uint32_t max_size_in_bytes_;
+	uint32_t size_in_bytes_;
+	std::map<std::string, Entry> entries_;
+	AccessHistory access_history_;
+
 	DISALLOW_COPY_AND_ASSIGN(TextureCache);
 };
 
-// Create a new Cache whichs combined pixels in all transient surfaces are
-// always below the given limit (Note: there is overhead for class members
-// which is not counted as the pixels make up the bulk of the size of a
-// surface).
-TextureCache* create_texture_cache(uint32_t transient_memory_in_bytes);
-
 #endif  // end of include guard: WL_GRAPHIC_TEXTURE_CACHE_H