From d133ea6abba7dbe6305e580a35f01e79904a2ba4 Mon Sep 17 00:00:00 2001
From: Alan Wu <XrXr@users.noreply.github.com>
Date: Mon, 26 Nov 2018 11:08:09 -0500
Subject: [PATCH 2/2] Hash code memoization for short fstrings

For short enough fstrings, we have extra space inside the
RString struct we could use to remember the hash code.

Applications that work with JSON or YAML benefit from this
optimization as they often use string literals as hash keys.
The popular ORM ActiveRecord also uses fstring internally and
thus would benefit from this optimization.

This also provides yet another incentive for users to turn on
frozen string literals.

On a 64-bit machine, for strings that use ASCII and UTF-8
encoding, "short enough" means len < 16. For 32 bit machines,
"short enough" means len < 8.

* string.c: memoize hash code for short, embeded fstrings after
  the embedded buffer. Make use of st_update_with_hash() to
  avoid hashing the same string multiple times.

* string.c: harden str_fill_term() to make sure we don't zero
  more than 4 bytes of memory. No encoding in the world needs
  a terminator longer than 4 bytes at the moment.

* hash_aref_fstr.rb: benchmark demonstrating the best case
  scenario. About 20% faster with this commit.

* hash_aref_long_str.rb: benchamrk demonstrating the worst case
  scenario for this optimization. It shows that there is no
  visible performance impact for strings that don't benefit
  from the optimization.

* freeze_unique_strings.yml: benchmark that creates many unique
  fstrings. We memoize the hash for each fstring we create, but
  there is no visible performance impact.
---
 benchmark/freeze_unique_strings.yml |  7 +++
 benchmark/hash_aref_fstr.rb         |  4 ++
 benchmark/hash_aref_long_str.rb     |  4 ++
 ext/-test-/string/cstr.c            |  1 +
 internal.h                          |  1 +
 string.c                            | 72 ++++++++++++++++++++++-------
 test/-ext-/string/test_cstr.rb      | 17 +++++++
 7 files changed, 90 insertions(+), 16 deletions(-)
 create mode 100644 benchmark/freeze_unique_strings.yml
 create mode 100644 benchmark/hash_aref_fstr.rb
 create mode 100644 benchmark/hash_aref_long_str.rb

diff --git a/benchmark/freeze_unique_strings.yml b/benchmark/freeze_unique_strings.yml
new file mode 100644
index 0000000000..0f669102d2
--- /dev/null
+++ b/benchmark/freeze_unique_strings.yml
@@ -0,0 +1,7 @@
+prelude: |
+  str = +"0000000"
+benchmark:
+  freeze_unique_strings: |
+    str.succ!
+    -str
+loop_count: 9000000 # freeze this many unique strings
diff --git a/benchmark/hash_aref_fstr.rb b/benchmark/hash_aref_fstr.rb
new file mode 100644
index 0000000000..33c6c4088b
--- /dev/null
+++ b/benchmark/hash_aref_fstr.rb
@@ -0,0 +1,4 @@
+h = {}
+strs = ('a'..'z').to_a.map!(&:-@)
+strs.each { |s| h[s] = s }
+500_000.times { strs.each { |s| h[s] } }
diff --git a/benchmark/hash_aref_long_str.rb b/benchmark/hash_aref_long_str.rb
new file mode 100644
index 0000000000..d96cf7fdb5
--- /dev/null
+++ b/benchmark/hash_aref_long_str.rb
@@ -0,0 +1,4 @@
+h = {}
+strs = ['a' * 100] * 10
+strs.each { |s| h[s] = s }
+200_000.times { strs.each { |s| h[s] } }
diff --git a/ext/-test-/string/cstr.c b/ext/-test-/string/cstr.c
index 71eafdb703..b050aaefb6 100644
--- a/ext/-test-/string/cstr.c
+++ b/ext/-test-/string/cstr.c
@@ -144,4 +144,5 @@ Init_string_cstr(VALUE klass)
     rb_define_singleton_method(klass, "cstr_noembed", bug_str_s_cstr_noembed, 1);
     rb_define_singleton_method(klass, "cstr_embedded?", bug_str_s_cstr_embedded_p, 1);
     rb_define_singleton_method(klass, "rb_str_new_frozen", bug_str_s_rb_str_new_frozen, 1);
+    rb_define_const(klass, "HASH_MEMO_STR_BUFFER_CAPA", INT2NUM(HASH_MEMO_STR_BUFFER_CAPA));
 }
diff --git a/internal.h b/internal.h
index d36723b273..e25b3b4350 100644
--- a/internal.h
+++ b/internal.h
@@ -2041,6 +2041,7 @@ VALUE rb_str_initialize(VALUE str, const char *ptr, long len, rb_encoding *enc);
 #define STR_SHARED_P(s)  FL_ALL_RAW((s), STR_NOEMBED|ELTS_SHARED)
 #define is_ascii_string(str) (rb_enc_str_coderange(str) == ENC_CODERANGE_7BIT)
 #define is_broken_string(str) (rb_enc_str_coderange(str) == ENC_CODERANGE_BROKEN)
+#define HASH_MEMO_STR_BUFFER_CAPA ((int) (SIZEOF_VALUE * 2))
 size_t rb_str_memsize(VALUE);
 VALUE rb_sym_proc_call(ID mid, int argc, const VALUE *argv, VALUE passed_proc);
 VALUE rb_sym_to_proc(VALUE sym);
diff --git a/string.c b/string.c
index d7ef9cc9bc..3b11f0ad98 100644
--- a/string.c
+++ b/string.c
@@ -100,6 +100,24 @@ VALUE rb_cSymbol;
     RBASIC(str)->flags |= (tmp_n) << RSTRING_EMBED_LEN_SHIFT;\
 } while (0)
 
+struct memoized_hash_embeded_str {
+    char ary[HASH_MEMO_STR_BUFFER_CAPA];
+    st_index_t memoized_hash;
+};
+
+/* We need to have enough space for the string plus its encoding dependent terminator.
+   There are encodings that use two bytes, but we save on fetching the encoding
+   struct by being conservative. Change this when there is an encoding with 8-byte units.
+   See encindex.h.  */
+#define SHORT_ENOUGH_TO_MEMO_HASH(str, enc_idx) \
+    ((RSTRING_EMBED_LEN(str) + ((enc_idx <= ENCINDEX_US_ASCII) ? 1 : 4)) <= HASH_MEMO_STR_BUFFER_CAPA)
+STATIC_ASSERT(memoized_hash_type_punning, sizeof(struct memoized_hash_embeded_str) == sizeof(RSTRING(0)->as));
+#define SET_HASH_MEMO(str, hash) do { \
+    struct RString *rstr = RSTRING(str);\
+    ((struct memoized_hash_embeded_str*) &rstr->as)->memoized_hash = hash;\
+} while (0)
+#define GET_HASH_MEMO(str) (((struct memoized_hash_embeded_str*) &RSTRING(str)->as)->memoized_hash)
+
 #define STR_SET_LEN(str, n) do { \
     if (STR_EMBED_P(str)) {\
 	STR_SET_EMBED_LEN((str), (n));\
@@ -260,12 +278,17 @@ const struct st_hash_type rb_fstring_hash_type = {
     rb_str_hash,
 };
 
+struct fstr_update {
+    VALUE ret;
+    st_index_t hash;
+};
+
 #define BARE_STRING_P(str) (!FL_ANY_RAW(str, FL_TAINT|FL_EXIVAR) && RBASIC_CLASS(str) == rb_cString)
 
 static int
 fstr_update_callback(st_data_t *key, st_data_t *value, st_data_t arg, int existing)
 {
-    VALUE *fstr = (VALUE *)arg;
+    struct fstr_update *update = (struct fstr_update *)arg;
     VALUE str = (VALUE)*key;
 
     if (existing) {
@@ -273,11 +296,11 @@ fstr_update_callback(st_data_t *key, st_data_t *value, st_data_t arg, int existi
 	 * at next time */
 
 	if (rb_objspace_garbage_object_p(str)) {
-	    *fstr = Qundef;
+	    update->ret = Qundef;
 	    return ST_DELETE;
 	}
 
-	*fstr = str;
+	update->ret = str;
 	return ST_STOP;
     }
     else {
@@ -298,9 +321,13 @@ fstr_update_callback(st_data_t *key, st_data_t *value, st_data_t arg, int existi
 		str = str_new_frozen(rb_cString, str);
 	    }
 	}
+        if (STR_EMBED_P(str) && SHORT_ENOUGH_TO_MEMO_HASH(str, ENCODING_GET_INLINED(str))) {
+            SET_HASH_MEMO(str, update->hash);
+        }
 	RBASIC(str)->flags |= RSTRING_FSTR;
 
-	*key = *value = *fstr = str;
+	*key = *value = str;
+        update->ret = str;
 	return ST_CONTINUE;
     }
 }
@@ -333,24 +360,30 @@ rb_fstring(VALUE str)
     return fstr;
 }
 
+int
+st_update_with_hash(st_table *tab, st_data_t key, st_index_t hash,
+                    st_update_callback_func *func, st_data_t arg);  /* st.c */
+
 static VALUE
 register_fstring(VALUE str)
 {
-    VALUE ret;
+    struct fstr_update update;
     st_table *frozen_strings = rb_vm_fstring_table();
+    st_index_t hash = rb_str_hash(str);
+    update.hash = hash;
 
     do {
-	ret = str;
-	st_update(frozen_strings, (st_data_t)str,
-		  fstr_update_callback, (st_data_t)&ret);
-    } while (ret == Qundef);
-
-    assert(OBJ_FROZEN(ret));
-    assert(!FL_TEST_RAW(ret, STR_FAKESTR));
-    assert(!FL_TEST_RAW(ret, FL_EXIVAR));
-    assert(!FL_TEST_RAW(ret, FL_TAINT));
-    assert(RBASIC_CLASS(ret) == rb_cString);
-    return ret;
+        update.ret = str;
+        st_update_with_hash(frozen_strings, (st_data_t)str, hash,
+                            fstr_update_callback, (st_data_t)&update);
+    } while (update.ret == Qundef);
+
+    assert(OBJ_FROZEN(update.ret));
+    assert(!FL_TEST_RAW(update.ret, STR_FAKESTR));
+    assert(!FL_TEST_RAW(update.ret, FL_EXIVAR));
+    assert(!FL_TEST_RAW(update.ret, FL_TAINT));
+    assert(RBASIC_CLASS(update.ret) == rb_cString);
+    return update.ret;
 }
 
 static VALUE
@@ -2190,6 +2223,9 @@ str_fill_term(VALUE str, char *s, long len, int termlen)
 	    str_make_independent_expand(str, len, 0L, termlen);
     }
     else {
+        if (UNLIKELY(termlen > 4)) {
+            rb_raise(rb_eArgError, "terminator is longer than 4 bytes");
+        }
 	TERM_FILL(s + len, termlen);
 	return s;
     }
@@ -2202,6 +2238,7 @@ rb_str_change_terminator_length(VALUE str, const int oldtermlen, const int terml
     long capa = str_capacity(str, oldtermlen) + oldtermlen;
     long len = RSTRING_LEN(str);
 
+    assert(termlen <= 4);
     assert(capa >= len);
     if (capa - len < termlen) {
 	rb_check_lockedtmp(str);
@@ -3134,6 +3171,9 @@ st_index_t
 rb_str_hash(VALUE str)
 {
     int e = ENCODING_GET(str);
+    if (FL_TEST_RAW(str, RSTRING_FSTR | RSTRING_NOEMBED) == RSTRING_FSTR && SHORT_ENOUGH_TO_MEMO_HASH(str, e)) {
+        return GET_HASH_MEMO(str);
+    }
     if (e && rb_enc_str_coderange(str) == ENC_CODERANGE_7BIT) {
 	e = 0;
     }
diff --git a/test/-ext-/string/test_cstr.rb b/test/-ext-/string/test_cstr.rb
index 2083016db2..c6d0681ff5 100644
--- a/test/-ext-/string/test_cstr.rb
+++ b/test/-ext-/string/test_cstr.rb
@@ -50,6 +50,23 @@ def test_rb_str_new_frozen_embed
 
   WCHARS = [Encoding::UTF_16BE, Encoding::UTF_16LE, Encoding::UTF_32BE, Encoding::UTF_32LE]
 
+  def test_fstring_hash_memo_wchar
+    WCHARS.each_with_index do |encoding|
+      (1..Bug::String::HASH_MEMO_STR_BUFFER_CAPA).each do |i|
+        contrived_string = +('8' * i)
+        contrived_string.force_encoding(encoding)
+        before = contrived_string.hash
+
+        fstring = -(contrived_string)
+        assert Bug::String.cstr_embedded?(fstring)
+        # StringValueCStr writes a terminator past the end of the string buffer,
+        # which could clobber the memoized hash.
+        Bug::String.cstr_term(fstring)
+        assert_equal(before, fstring.hash)
+      end
+    end
+  end
+
   def test_wchar_embed
     WCHARS.each do |enc|
       s = Bug::String.new("\u{4022}a".encode(enc))
-- 
2.19.1

