From fae55ba56a6b22b95e838460a893d20f1662f6b4 Mon Sep 17 00:00:00 2001
From: James Roseborough <james.roseborough@luaj.org>
Date: Sat, 26 Jan 2013 19:01:00 +0000
Subject: [PATCH] Fix bug 3597515 memory leak due to string caching by
 simplifying caching logic.

---
 README.html                             |  3 +
 src/core/org/luaj/vm2/LuaString.java    | 65 +++++++++++-------
 test/junit/org/luaj/vm2/StringTest.java | 87 +++++++++++++++++++++++--
 3 files changed, 129 insertions(+), 26 deletions(-)

diff --git a/README.html b/README.html
index bfc2e2ba..efed2652 100644
--- a/README.html
+++ b/README.html
@@ -819,6 +819,9 @@ and LuaForge:
 <tr valign="top"><td>&nbsp;&nbsp;<b>3.0-alpha2</b></td><td><ul>
 <li>Supply environment as second argument to LibFunction when loading via require() </li>
 
+<tr valign="top"><td>&nbsp;&nbsp;<b>3.0-alpha3</b></td><td><ul>
+<li>Fix bug 3597515 memory leak due to string caching by simplifying caching logic.</li>
+
 </ul></td></tr>
 </table></td></tr></table>
 
diff --git a/src/core/org/luaj/vm2/LuaString.java b/src/core/org/luaj/vm2/LuaString.java
index 77d1b4cf..db4d4891 100644
--- a/src/core/org/luaj/vm2/LuaString.java
+++ b/src/core/org/luaj/vm2/LuaString.java
@@ -60,6 +60,22 @@ import org.luaj.vm2.lib.StringLib;
  */
 public class LuaString extends LuaValue {
 
+	/** Size of cache of recent short strings. This is the maximum number of LuaStrings that 
+	 * will be retained in the cache of recent short strings.  */
+	public static final int RECENT_STRINGS_CACHE_SIZE = 128;
+
+	/** Maximum length of a string to be considered for recent short strings caching. 
+	 * This effectively limits the total memory that can be spent on the recent strings cache,
+	 * ecause no LuaString whose backing exceeds this length will be put into the cache.  */
+	public static final int RECENT_STRINGS_MAX_LENGTH = 32;
+
+	/** Simple cache of recently created strings that are short.  
+	 * This is simply a list of strings, indexed by their hash codes modulo the cache size 
+	 * that have been recently constructed.  If a string is being constructed frequently 
+	 * from different contexts, it will generally may show up as a cache hit and resolve 
+	 * to the same value.  */
+	public static volatile LuaString recent_short_strings[] = new LuaString[RECENT_STRINGS_CACHE_SIZE];
+
 	/** The singleton instance representing lua {@code true} */
 	public static LuaValue s_metatable;
 
@@ -71,18 +87,7 @@ public class LuaString extends LuaValue {
 	
 	/** The number of bytes that comprise this string */
 	public final int    m_length;
-
-	private static final Hashtable index_java = new Hashtable();
-
-	private final static LuaString index_get(Hashtable indextable, Object key) {
-		WeakReference w = (WeakReference) indextable.get(key);
-		return w!=null? (LuaString) w.get(): null;
-	}
 	
-	private final static void index_set(Hashtable indextable, Object key, LuaString value) {
-		indextable.put(key, new WeakReference(value));
-	}
-
 	/**
 	 * Get a {@link LuaString} instance whose bytes match 
 	 * the supplied Java String using the UTF8 encoding. 
@@ -90,28 +95,44 @@ public class LuaString extends LuaValue {
 	 * @return {@link LuaString} with UTF8 bytes corresponding to the supplied String
 	 */
 	public static LuaString valueOf(String string) {
-		LuaString s = index_get( index_java, string );
-		if ( s != null ) return s;
 		char[] c = string.toCharArray();
 		byte[] b = new byte[lengthAsUtf8(c)];
 		encodeToUtf8(c, b, 0);
-		s = valueOf(b, 0, b.length);
-		index_set( index_java, string, s );
-		return s;
+		return valueOf(b, 0, b.length);
 	}
 
 	// TODO: should this be deprecated or made private?
-	/** Construct a {@link LuaString} around a byte array without copying the contents.
+	/** Construct a {@link LuaString} around a byte array that may be used directly as the backing.
 	 * <p>
-	 * The array is used directly after this is called, so clients must not change contents.
+	 * The array may be used as the backing for this object, so clients must not change contents.
+	 * If the supplied value for 'len' is more than half the length of the container, the 
+	 * supplied byte array will be used as the backing, otherwise the bytes will be copied to a
+	 * new byte array, and cache lookup may be performed.
 	 * <p>
 	 * @param bytes byte buffer
 	 * @param off offset into the byte buffer
 	 * @param len length of the byte buffer
 	 * @return {@link LuaString} wrapping the byte buffer
 	 */
-	public static LuaString valueOf(byte[] bytes, int off, int len) { 
-		return new LuaString(bytes, off, len);
+	public static LuaString valueOf(byte[] bytes, int off, int len) {
+		if (bytes.length < RECENT_STRINGS_MAX_LENGTH) {
+			// Short string.  Reuse the backing and check the cache of recent strings before returning.
+			final LuaString s = new LuaString(bytes, off, len);
+			final int index = s.hashCode() & (RECENT_STRINGS_CACHE_SIZE - 1);
+			final LuaString cached = recent_short_strings[index];
+			if (cached != null && s.raweq(cached))
+				return cached;
+			recent_short_strings[index] = s;
+			return s;
+		} else if (len >= bytes.length / 2) {
+			// Reuse backing only when more than half the bytes are part of the result.
+			return new LuaString(bytes, off, len);
+		} else {
+			// Short result relative to the source.  Copy only the bytes that are actually to be used.
+			final byte[] b = new byte[len];
+			System.arraycopy(bytes, off, b, 0, len);
+			return valueOf(bytes);
+		}
 	}
 	
 	/** Construct a {@link LuaString} using the supplied characters as byte values.
@@ -127,13 +148,13 @@ public class LuaString extends LuaValue {
 		byte[] b = new byte[n];
 		for ( int i=0; i<n; i++ )
 			b[i] = (byte) bytes[i];
-		return valueOf(b, 0, n);
+		return valueOf(b, 0, b.length);
 	}
 	
 	
 	/** Construct a {@link LuaString} around a byte array without copying the contents.
 	 * <p>
-	 * The array is used directly after this is called, so clients must not change contents.
+	 * The array may be used directly as the backing, so clients must not change contents.
 	 * <p>
 	 * @param bytes byte buffer
 	 * @return {@link LuaString} wrapping the byte buffer
diff --git a/test/junit/org/luaj/vm2/StringTest.java b/test/junit/org/luaj/vm2/StringTest.java
index b2b2f612..ede35adf 100644
--- a/test/junit/org/luaj/vm2/StringTest.java
+++ b/test/junit/org/luaj/vm2/StringTest.java
@@ -74,7 +74,6 @@ public class StringTest extends TestCase {
 		LuaString ls = LuaString.valueOf(before);
 		String after = ls.tojstring();
 		assertEquals( userFriendly( before ), userFriendly( after ) );
-		
 	}
 	
 	public void testUtf8() {		
@@ -90,7 +89,6 @@ public class StringTest extends TestCase {
 		LuaString ls = LuaString.valueOf(before);
 		String after = ls.tojstring();
 		assertEquals( userFriendly( before ), userFriendly( after ) );
-		
 	}
 
 	public void testSpotCheckUtf8() throws UnsupportedEncodingException {
@@ -103,7 +101,7 @@ public class StringTest extends TestCase {
 		assertEquals(162, d[2]);
 		assertEquals(163, d[3]);
 		assertEquals(164, d[4]);
-		
+		assertEquals(expected, actual);
 	}
 	
 	public void testNullTerminated() {		
@@ -112,6 +110,87 @@ public class StringTest extends TestCase {
 		LuaString ls = LuaString.valueOf(before);
 		String after = ls.tojstring();
 		assertEquals( userFriendly( "abc\0def" ), userFriendly( after ) );
-		
+	}
+
+	public void testRecentStringsCacheDifferentHashcodes() {
+		final byte[] abc = {'a', 'b', 'c' };
+		final byte[] xyz = {'x', 'y', 'z' };
+		final LuaString abc1 = LuaString.valueOf(abc);
+		final LuaString xyz1 = LuaString.valueOf(xyz);
+		final LuaString abc2 = LuaString.valueOf(abc);
+		final LuaString xyz2 = LuaString.valueOf(xyz);
+		final int mod = LuaString.RECENT_STRINGS_CACHE_SIZE;
+		assertTrue(abc1.hashCode() % mod != xyz1.hashCode() % mod);
+		assertSame(abc1, abc2);
+		assertSame(xyz1, xyz2);
+	}
+
+	public void testRecentStringsCacheHashCollisionCacheHit() {
+		final byte[] abc = {'a', 'b', 'c' };
+		final byte[] lyz = {'l', 'y', 'z' };  // chosen to have hash collision with 'abc'
+		final LuaString abc1 = LuaString.valueOf(abc);
+		final LuaString abc2 = LuaString.valueOf(abc); // in cache: 'abc'
+		final LuaString lyz1 = LuaString.valueOf(lyz);
+		final LuaString lyz2 = LuaString.valueOf(lyz); // in cache: 'lyz'
+		final int mod = LuaString.RECENT_STRINGS_CACHE_SIZE;
+		assertEquals(abc1.hashCode() % mod, lyz1.hashCode() % mod);
+		assertNotSame(abc1, lyz1);
+		assertFalse(abc1.equals(lyz1));
+		assertSame(abc1, abc2);
+		assertSame(lyz1, lyz2);
+	}
+
+	public void testRecentStringsCacheHashCollisionCacheMiss() {
+		final byte[] abc = {'a', 'b', 'c' };
+		final byte[] lyz = {'l', 'y', 'z' };  // chosen to have hash collision with 'abc'
+		final LuaString abc1 = LuaString.valueOf(abc);
+		final LuaString lyz1 = LuaString.valueOf(lyz); // in cache: 'abc'
+		final LuaString abc2 = LuaString.valueOf(abc); // in cache: 'lyz'
+		final LuaString lyz2 = LuaString.valueOf(lyz); // in cache: 'abc'
+		final int mod = LuaString.RECENT_STRINGS_CACHE_SIZE;
+		assertEquals(abc1.hashCode() % mod, lyz1.hashCode() % mod);
+		assertNotSame(abc1, lyz1);
+		assertFalse(abc1.equals(lyz1));
+		assertNotSame(abc1, abc2);
+		assertNotSame(lyz1, lyz2);
+	}
+
+	public void testRecentStringsLongStrings() {
+		byte[] abc = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ".getBytes();
+		assertTrue(abc.length > LuaString.RECENT_STRINGS_MAX_LENGTH);
+		LuaString abc1 = LuaString.valueOf(abc);
+		LuaString abc2 = LuaString.valueOf(abc);
+		assertNotSame(abc1, abc2);
+	}
+
+	public void testRecentStringsUsingJavaStrings() {
+		final String abc = "abc";
+		final String lyz = "lyz";  // chosen to have hash collision with 'abc'
+		final String xyz = "xyz";
+
+		final LuaString abc1 = LuaString.valueOf(abc);
+		final LuaString abc2 = LuaString.valueOf(abc);
+		final LuaString lyz1 = LuaString.valueOf(lyz);
+		final LuaString lyz2 = LuaString.valueOf(lyz);
+		final LuaString xyz1 = LuaString.valueOf(xyz);
+		final LuaString xyz2 = LuaString.valueOf(xyz);
+		final int mod = LuaString.RECENT_STRINGS_CACHE_SIZE;
+		assertEquals(abc1.hashCode() % mod, lyz1.hashCode() % mod);
+		assertFalse(abc1.hashCode() % mod == xyz1.hashCode() % mod);
+		assertSame(abc1, abc2);
+		assertSame(lyz1, lyz2);
+		assertSame(xyz1, xyz2);
+
+		final LuaString abc3 = LuaString.valueOf(abc);
+		final LuaString lyz3 = LuaString.valueOf(lyz);
+		final LuaString xyz3 = LuaString.valueOf(xyz);
+
+		final LuaString abc4 = LuaString.valueOf(abc);
+		final LuaString lyz4 = LuaString.valueOf(lyz);
+		final LuaString xyz4 = LuaString.valueOf(xyz);
+		assertNotSame(abc3, abc4);  // because of hash collision
+		assertNotSame(lyz3, lyz4);  // because of hash collision
+		assertSame(xyz3, xyz4);  // because hashes do not collide
+
 	}
 }