Fix fz_store thinko.

Previously, we would refuse to store any object in the store that
was larger than the store limits. We'd also refuse to store any
object that took the total store size over the limit.

This was wrong.

Consider the case where we have a store of 1 byte, and a page that
repeatedly uses the same font. The first time we meet the font, we
look in the store, it isn't there, we load it, and we try to store
it. The current code refuses to store it, and we continue, putting
that font into the display list.

The next time we meet to the font, we look in the store, it still
isn't there, we load it, and we try to store it. Again we refuse to
store it, and that copy of the font goes into the display list.

The net effect of this is that we end up using far more memory in
total than we would have done had we stored the first one.

The code here, therefore, changes the store to always store objects
regardless of their size. Given that we have already loaded the
objects into memory before we store them, this doesn't actually
cost us any extra memory. If an object is dropped (bringing the
reference count down to 1, being the reference for the stores copy),
then the object is NOT freed instantly, but will be freed either
on the next attempt to store an object, or on the next scavenging
malloc.

1 files changed, 11 insertions, 26 deletions

diff --git a/source/fitz/store.c b/source/fitz/store.c
index a71fb94d..c80e1087 100644
--- a/source/fitz/store.c
+++ b/source/fitz/store.c
@@ -223,13 +223,6 @@ fz_store_item(fz_context *ctx, void *key, void *val_, unsigned int itemsize, fz_
 
 	fz_var(item);
 
-	if (store->max != FZ_STORE_UNLIMITED && store->max < itemsize)
-	{
-		/* Our item would take up more room than we can ever
-		 * possibly have in the store. Just give up now. */
-		return NULL;
-	}
-
 	/* If we fail for any reason, we swallow the exception and continue.
 	 * All that the above program will see is that we failed to store
 	 * the item. */
@@ -306,29 +299,21 @@ fz_store_item(fz_context *ctx, void *key, void *val_, unsigned int itemsize, fz_
 		{
 			/* ensure_space may drop, then retake the lock */
 			int saved = ensure_space(ctx, size - store->max);
+			size -= saved;
 			if (saved == 0)
 			{
 				/* Failed to free any space. */
-				/* If we are using the hash table, then we've already
-				 * inserted item - remove it. */
-				if (use_hash)
-				{
-					/* If someone else has already picked up a reference
-					 * to item, then we cannot remove it. Leave it in the
-					 * store, and we'll live with being over budget. We
-					 * know this is the case, if it's in the linked list. */
-					if (item->next != item)
-						break;
-					fz_hash_remove_fast(ctx, store->hash, &hash, pos);
-				}
-				fz_unlock(ctx, FZ_LOCK_ALLOC);
-				fz_free(ctx, item);
-				type->drop_key(ctx, key);
-				if (val->refs > 0)
-					val->refs--;
-				return NULL;
+				/* We used to 'unstore' it here, but that's wrong.
+				 * If we've already spent the memory to malloc it
+				 * then not putting it in the store just means that
+				 * a resource used multiple times will just be malloced
+				 * again. Better to put it in the store, have the
+				 * store account for it, and for it to potentially be reused.
+				 * When the caller drops the reference to it, it can then
+				 * be dropped from the store on the next attempt to store
+				 * anything else. */
+				break;
 			}
-			size -= saved;
 		}
 	}
 	store->size += itemsize;