Project

General

Profile

Actions

Feature #9171

closed

[patch] use fstrings for symbol table

Added by tmm1 (Aman Karmani) almost 11 years ago. Updated almost 9 years ago.

Status:
Closed
Target version:
-
[ruby-core:58656]

Description

Here is a simple patch to use fstrings for the table backing symbols.

Unfortunately it causes a segfault in test/rdoc/test_rdoc_parser_ruby.rb. Maybe someone wants to investigate.

diff --git a/parse.y b/parse.y
index 8207ad7..b1f3112 100644
--- a/parse.y
+++ b/parse.y
@@ -10333,7 +10333,7 @@ register_symid(ID id, const char *name, long len, rb_encoding *enc)
 static ID
 register_symid_str(ID id, VALUE str)
 {
-    OBJ_FREEZE(str);
+    str = rb_fstring(str);
 
     if (RUBY_DTRACE_SYMBOL_CREATE_ENABLED()) {
 	RUBY_DTRACE_SYMBOL_CREATE(RSTRING_PTR(str), rb_sourcefile(), rb_sourceline());
diff --git a/string.c b/string.c
index 231bb2f..6ae33e3 100644
--- a/string.c
+++ b/string.c
@@ -138,6 +138,9 @@ rb_fstring(VALUE str)
     st_data_t fstr;
     Check_Type(str, T_STRING);
 
+    if (!frozen_strings)
+	frozen_strings = st_init_table(&fstring_hash_type);
+
     if (FL_TEST(str, RSTRING_FSTR))
 	return str;
 
@@ -8707,8 +8710,6 @@ Init_String(void)
 #undef rb_intern
 #define rb_intern(str) rb_intern_const(str)
 
-    frozen_strings = st_init_table(&fstring_hash_type);
-
     rb_cString  = rb_define_class("String", rb_cObject);
     rb_include_module(rb_cString, rb_mComparable);
     rb_define_alloc_func(rb_cString, empty_str_alloc);
Actions #1

Updated by hsbt (Hiroshi SHIBATA) almost 11 years ago

  • Assignee set to ko1 (Koichi Sasada)

ko1: Could you review this?

Updated by ko1 (Koichi Sasada) almost 11 years ago

  • Category set to core
  • Assignee changed from ko1 (Koichi Sasada) to nobu (Nobuyoshi Nakada)

Nobu, could you check it?

Updated by avit (Andrew Vit) almost 11 years ago

Would this allow the symbol table to be GC'd or is that a separate issue?

Updated by nobu (Nobuyoshi Nakada) almost 11 years ago

Seems my reply hasn't caught by the ITS...

(13/11/28 16:55), tmm1 (Aman Gupta) wrote:

diff --git a/parse.y b/parse.y
index 8207ad7..b1f3112 100644
--- a/parse.y
+++ b/parse.y
@@ -10333,7 +10333,7 @@ register_symid(ID id, const char *name, long len, rb_encoding *enc)
 static ID
 register_symid_str(ID id, VALUE str)
 {
-    OBJ_FREEZE(str);
+    str = rb_fstring(str);

At this point, rb_cString is not created yet.
And unfrozen string will be duplicated in rb_fstring().

diff --git i/parse.y w/parse.y
index 8207ad7..9f580e6 100644
--- i/parse.y
+++ w/parse.y
@@ -10334,6 +10334,7 @@ static ID
 register_symid_str(ID id, VALUE str)
 {
     OBJ_FREEZE(str);
+    str = rb_fstring(str);
 
     if (RUBY_DTRACE_SYMBOL_CREATE_ENABLED()) {
 	RUBY_DTRACE_SYMBOL_CREATE(RSTRING_PTR(str), rb_sourcefile(), rb_sourceline());
diff --git i/string.c w/string.c
index 151170c..ec43af7 100644
--- i/string.c
+++ w/string.c
@@ -165,6 +165,9 @@ rb_fstring(VALUE str)
     VALUE fstr = Qnil;
     Check_Type(str, T_STRING);
 
+    if (!frozen_strings)
+	frozen_strings = st_init_table(&fstring_hash_type);
+
     if (FL_TEST(str, RSTRING_FSTR))
 	return str;
 
@@ -173,6 +176,13 @@ rb_fstring(VALUE str)
 }
 
 static int
+fstring_set_class_i(st_data_t key, st_data_t val, st_data_t arg)
+{
+    RBASIC_SET_CLASS((VALUE)key, (VALUE)arg);
+    return ST_CONTINUE;
+}
+
+static int
 fstring_cmp(VALUE a, VALUE b)
 {
     int cmp = rb_str_hash_cmp(a, b);
@@ -8718,8 +8728,6 @@ Init_String(void)
 #undef rb_intern
 #define rb_intern(str) rb_intern_const(str)
 
-    frozen_strings = st_init_table(&fstring_hash_type);
-
     rb_cString  = rb_define_class("String", rb_cObject);
     rb_include_module(rb_cString, rb_mComparable);
     rb_define_alloc_func(rb_cString, empty_str_alloc);
@@ -8891,4 +8899,6 @@ Init_String(void)
     rb_define_method(rb_cSymbol, "swapcase", sym_swapcase, 0);
 
     rb_define_method(rb_cSymbol, "encoding", sym_encoding, 0);
+
+    st_foreach(frozen_strings, fstring_set_class_i, rb_cString);
 }

Updated by tmm1 (Aman Karmani) almost 11 years ago

Ah great! Thanks for finding my bug.

With your patch, long-lived strings on our rails app heap are reduced by 6000. I think we should merge it.

Actions #7

Updated by tmm1 (Aman Karmani) almost 11 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r44057.
Aman, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


parse.y: use rb_fstring() for strings stored in the symbol table

  • parse.y (register_symid_str): use fstrings in symbol table
    [Bug #9171] [ruby-core:58656]
  • parse.y (rb_id2str): ditto
  • string.c (rb_fstring): create frozen_strings on first usage. this
    allows rb_fstring() calls from the parser (before cString is created)
  • string.c (fstring_set_class_i): set klass on fstrings generated
    before cString was defined
  • string.c (Init_String): convert frozen_strings table to String
    objects after boot
  • ext/-test-/symbol/type.c (bug_sym_id2str): expose rb_id2str()
  • test/-ext-/symbol/test_type.rb (module Test_Symbol): verify symbol
    table entries are fstrings

Updated by nobu (Nobuyoshi Nakada) almost 9 years ago

  • Tracker changed from Bug to Feature
  • Description updated (diff)
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0