Skip to content

Commit

Permalink
Use "interned" Strings when possible to avoid uneccessary heap allo…
Browse files Browse the repository at this point in the history
…cation.

Fixes ohler55#275

- Background info: https://en.wikipedia.org/wiki/String_interning
- Available to C extensions since MRI Ruby 3.0:
  - https://bugs.ruby-lang.org/issues/13381
  - https://bugs.ruby-lang.org/issues/16029

This change makes `Ox` use "interned" (frozen and deduplicated) `String`s anywhere possible,
the same effect as calling `String#-@` except without the heap churn of `RVALUE` allocation:
https://ruby-doc.org/core/String.html#method-i-2B-40

- Uses the `HAVE_<funcname>` preprocessor macros to detect this functionality
  and avoid breaking older Rubies (confirmed with at least MRI 2.7.2).
- I explicitly use interned `String`s anywhere an allocated `String` seemed
  entirely internal to `Ox`, e.g. in `sax_value_as_time` when allocating
  the `String` argument to `ox_time_class`.
- Adds a new user-facing option `intern_strings` to control the behavior
  of `String` return values from user-facing functions, e.g. from `sax_as_s`.
- Results in a ~25% reduction in startup GC pressure in my library! :D

Inspired by similar changes to `json` and `msgpack`:

- flori/json#451
- msgpack/msgpack-ruby#196

My goal is `Object` allocation reduction in the `Ox::Sax` handler of my MIME Type
file-identification library caused by the large number of duplicate `String`s in the
`shared-mime-info`-format XML it uses as a data source.

I was already calling `#-@` (or using the `-some_str_variable` syntax) everywhere possible
in my handler, but that only freezes and deduplicates an already-allocated `String`:

  irb(main):068:0> oid = proc { puts "#{_1} is object_id #{_1.object_id}" }
  => #<Proc:0x0000564d53eb6850 (irb):66>
  irb(main):069:0> lol = 'lol'.tap(&oid).-@.tap(&oid)
  lol is object_id 19800
  lol is object_id 19740
  => "lol"
  irb(main):070:0> -lol.tap(&oid).-@.tap(&oid)
  lol is object_id 19740
  lol is object_id 19740
  => "lol"

That avoids duplicate retained `String`s but still causes my library to take
a big GC hit right at startup when it loads its data.

All the stats below were collected using SamSaffron's `memory_profiler`:
https://github.com/SamSaffron/memory_profiler

First, a sanity-check to make sure I didn't break MRI < 3.0,
using MRI 2.7 + latest official `Ox` from RubyGems (2.14.5):

  [okeeblow@emi#CHECKING-YOU-OUT] ruby -v
  ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-linux]
  [okeeblow@emi#CHECKING-YOU-OUT] ./bin/are-we-unallocated-yet|grep Total
  Total allocated: 20561351 bytes (401372 objects)
  Total retained:  1680971 bytes (22102 objects)

versus same MRI 2.7 but with my patched `Ox`:

  [okeeblow@emi#CHECKING-YOU-OUT] gem install ../../ox/ox-2.14.5.gem
  [okeeblow@emi#CHECKING-YOU-OUT] ./bin/are-we-unallocated-yet|grep Total
  Total allocated: 20561111 bytes (401366 objects)
  Total retained:  1680971 bytes (22102 objects)

No difference between the two, showing that this patch is a no-op for pre-3.0.
Sorry I did not test older versions of MRI or other Rubies like J/Truffle/etc
since I don't have them on my system.

Now the real gainz can be seen when comparing MRI 3.0 with unpatched `Ox`:

  [okeeblow@emi#CHECKING-YOU-OUT] bundle install
  Fetching gem metadata from https://rubygems.org/......
  …
  Installing ox 2.14.5 with native extensions
  Using checking-you-out 0.7.0 from source at `.`
  Bundle complete! 11 Gemfile dependencies, 19 gems now installed.
  [okeeblow@emi#CHECKING-YOU-OUT] ./bin/are-we-unallocated-yet|grep Total
  Total allocated: 20081080 bytes (390133 objects)
  Total retained:  1713209 bytes (22095 objects)

against same MRI 3.0 with my patched `Ox`:

  [okeeblow@emi#CHECKING-YOU-OUT] gem install ../../ox/ox-2.14.5.gem
  Building native extensions. This could take a while...
  Successfully installed ox-2.14.5
  Parsing documentation for ox-2.14.5
  unknown encoding name ""UTF-8"" for README.md, skipping
  Installing ri documentation for ox-2.14.5
  Done installing documentation for ox after 0 seconds
  1 gem installed
  [okeeblow@emi#CHECKING-YOU-OUT] ./bin/are-we-unallocated-yet|grep Total
  Total allocated: 17298804 bytes (322860 objects)
  Total retained:  1713202 bytes (22073 objects)

against same MRI 3.0, my patched `Ox`, and `Ox.parse_sax(intern_strings: true)`:

  [okeeblow@emi#CHECKING-YOU-OUT] ./bin/are-we-unallocated-yet|grep Total
  Total allocated: 16414938 bytes (301382 objects)
  Total retained:  1713226 bytes (22073 objects)

This shows that just having Ruby 3.0 available results in a huge win,
and opting into immutable `String`s from `Value#as_s` helps me even more!

Unit tests pass:

  [okeeblow@emi#test] ruby tests.rb
  Loaded suite tests
  Started
  .................
  16 tests, 40 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
  100% passed
  .....
  18 tests, 42 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
  100% passed
  ...............................................................................................................................
  151 tests, 249 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
  100% passed

Apologies for possible indentation issues in this patch. There seem to be lots of existing
lines with a mix of tabs and spaces — nbd, it happens — so I tried to match the surrounding
lines everywhere I made an addition to make sure things line up and look okay in `git diff`.

I only use `Ox` for Sax parsing, not for marshalling, so please scrutinize my changes
in `gen_load.c` and friends extra hard in case I omitted any `intern_strings` option
checks that could result in an unwary user getting an immutable `String` where
there wasn't one before. The one change I had to make to a `String#force_encoding`-using
test case is an example of what I want to avoid surprising anyone with.
  • Loading branch information
okeeblow committed Jul 20, 2021
1 parent ed271a3 commit b0d9a8e
Show file tree
Hide file tree
Showing 10 changed files with 470 additions and 16 deletions.
4 changes: 4 additions & 0 deletions ext/ox/extconf.rb
Expand Up @@ -38,6 +38,10 @@
have_func('rb_struct_alloc_noinit')
have_func('rb_obj_encoding')
have_func('rb_ivar_foreach')
have_func('rb_interned_str', 'ruby.h')
have_func('rb_interned_str_cstr', 'ruby.h')
have_func('rb_enc_interned_str', 'ruby.h')
have_func('rb_enc_interned_str_cstr', 'ruby.h')

have_header('ruby/st.h')
have_header('sys/uio.h')
Expand Down
171 changes: 165 additions & 6 deletions ext/ox/gen_load.c
Expand Up @@ -105,24 +105,56 @@ create_prolog_doc(PInfo pi, const char *target, Attr attrs) {
} else if (Yes == pi->options->sym_keys) {
#if HAVE_RB_ENC_ASSOCIATE
if (0 != pi->options->rb_enc) {
#if HAVE_RB_ENC_INTERNED_STR_CSTR
VALUE rstr = rb_enc_interned_str_cstr(attrs->name, pi->options->rb_enc);
#else
VALUE rstr = rb_str_new2(attrs->name);

rb_enc_associate(rstr, pi->options->rb_enc);
#endif
sym = rb_funcall(rstr, ox_to_sym_id, 0);
} else {
sym = ID2SYM(rb_intern(attrs->name));
}
sym = ID2SYM(rb_intern(attrs->name));
#endif // HAVE_RB_ENC_ASSOCIATE
#if HAVE_RB_INTERNED_STR_CSTR
if (Yes == pi->options->intern_strings) {
rb_hash_aset(ah, sym, rb_interned_str_cstr(attrs->value));
} else {
#endif
rb_hash_aset(ah, sym, rb_str_new2(attrs->value));
#if HAVE_RB_INTERNED_STR_CSTR
}
#endif
} else {
#if HAVE_RB_ENC_ASSOCIATE
#if HAVE_ENC_RB_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
if (Yes == pi->options->intern_strings) {
if (0 != pi->options->rb_enc) {
volatile VALUE rstr = rb_enc_interned_str_cstr(attrs->name, pi->options->rb_enc);
} else {
volatile VALUE rstr = rb_interned_str_cstr(attrs->name);
}
} else {
#endif
volatile VALUE rstr = rb_str_new2(attrs->name);

if (0 != pi->options->rb_enc) {
rb_enc_associate(rstr, pi->options->rb_enc);
}
#if HAVE_ENC_RB_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
}
#endif
#if HAVE_RB_INTERNED_STR_CSTR
if (Yes == pi->options->intern_strings) {
rb_hash_aset(ah, rstr, rb_interned_str_cstr(attrs->value));
} else {
#endif
rb_hash_aset(ah, rstr, rb_str_new2(attrs->value));
#if HAVE_RB_INTERNED_STR_CSTR
}
#endif
#endif
}
if (0 == strcmp("encoding", attrs->name)) {
Expand Down Expand Up @@ -193,12 +225,25 @@ nomode_instruct(PInfo pi, const char *target, Attr attrs, const char *content) {
static void
add_doctype(PInfo pi, const char *docType) {
VALUE n = rb_obj_alloc(ox_doctype_clas);
VALUE s = rb_str_new2(docType);
VALUE s;
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
if (Yes == pi->options->intern_strings) {
if (0 != pi->options->rb_enc) {
s = rb_enc_interned_str_cstr(docType, pi->options->rb_enc);
} else {
s = rb_interned_str_cstr(docType);
}
} else {
#endif
s = rb_str_new2(docType);

#if HAVE_RB_ENC_ASSOCIATE
if (0 != pi->options->rb_enc) {
rb_enc_associate(s, pi->options->rb_enc);
}
#endif
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
}
#endif
rb_ivar_set(n, ox_at_value_id, s);
if (helper_stack_empty(&pi->helpers)) { /* top level object */
Expand All @@ -210,12 +255,25 @@ add_doctype(PInfo pi, const char *docType) {
static void
add_comment(PInfo pi, const char *comment) {
VALUE n = rb_obj_alloc(ox_comment_clas);
VALUE s = rb_str_new2(comment);
VALUE s;
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
if (Yes == pi->options->intern_strings) {
if (0 != pi->options->rb_enc) {
s = rb_enc_interned_str_cstr(comment, pi->options->rb_enc);
} else {
s = rb_interned_str_cstr(comment);
}
} else {
#endif
s = rb_str_new2(comment);

#if HAVE_RB_ENC_ASSOCIATE
if (0 != pi->options->rb_enc) {
rb_enc_associate(s, pi->options->rb_enc);
}
#endif
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
}
#endif
rb_ivar_set(n, ox_at_value_id, s);
if (helper_stack_empty(&pi->helpers)) { /* top level object */
Expand All @@ -227,12 +285,25 @@ add_comment(PInfo pi, const char *comment) {
static void
add_cdata(PInfo pi, const char *cdata, size_t len) {
VALUE n = rb_obj_alloc(ox_cdata_clas);
VALUE s = rb_str_new2(cdata);
VALUE s;
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
if (Yes == pi->options->intern_strings) {
if (0 != pi->options->rb_enc) {
s = rb_enc_interned_str_cstr(cdata, pi->options->rb_enc);
} else {
s = rb_interned_str_cstr(cdata);
}
} else {
#endif
s = rb_str_new2(cdata);

#if HAVE_RB_ENC_ASSOCIATE
if (0 != pi->options->rb_enc) {
rb_enc_associate(s, pi->options->rb_enc);
}
#endif
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
}
#endif
rb_ivar_set(n, ox_at_value_id, s);
if (helper_stack_empty(&pi->helpers)) { /* top level object */
Expand All @@ -243,12 +314,25 @@ add_cdata(PInfo pi, const char *cdata, size_t len) {

static void
add_text(PInfo pi, char *text, int closed) {
VALUE s = rb_str_new2(text);
VALUE s;
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
if (Yes == pi->options->intern_strings) {
if (0 != pi->options->rb_enc) {
s = rb_enc_interned_str_cstr(text, pi->options->rb_enc);
} else {
s = rb_interned_str_cstr(text);
}
} else {
#endif
s = rb_str_new2(text);

#if HAVE_RB_ENC_ASSOCIATE
if (0 != pi->options->rb_enc) {
rb_enc_associate(s, pi->options->rb_enc);
}
#endif
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
}
#endif
if (helper_stack_empty(&pi->helpers)) { /* top level object */
create_doc(pi);
Expand Down Expand Up @@ -285,9 +369,13 @@ add_element(PInfo pi, const char *ename, Attr attrs, int hasChildren) {
if (Qundef == (sym = ox_cache_get(ox_symbol_cache, attrs->name, &slot, 0))) {
#if HAVE_RB_ENC_ASSOCIATE
if (0 != pi->options->rb_enc) {
#if HAVE_RB_ENC_INTERNED_STR_CSTR
VALUE rstr = rb_enc_interned_str_cstr(attrs->name, pi->options->rb_enc);
#else
VALUE rstr = rb_str_new2(attrs->name);

rb_enc_associate(rstr, pi->options->rb_enc);
#endif
sym = rb_funcall(rstr, ox_to_sym_id, 0);
} else {
sym = ID2SYM(rb_intern(attrs->name));
Expand All @@ -301,18 +389,42 @@ add_element(PInfo pi, const char *ename, Attr attrs, int hasChildren) {
*slot = sym;
}
} else {
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
if (Yes == pi->options->intern_strings) {
if (0 != pi->options->rb_enc) {
sym = rb_enc_interned_str_cstr(attrs->name, pi->options->rb_enc);
} else {
sym = rb_interned_str_cstr(attrs->name);
}
} else {
#endif
sym = rb_str_new2(attrs->name);
#if HAVE_RB_ENC_ASSOCIATE
if (0 != pi->options->rb_enc) {
rb_enc_associate(sym, pi->options->rb_enc);
}
#endif
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
}
#endif
}
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
if (Yes == pi->options->intern_strings) {
if (0 != pi->options->rb_enc) {
s = rb_enc_interned_str_cstr(attrs->value, pi->options->rb_enc);
} else {
s = rb_interned_str_cstr(attrs->value);
}
} else {
#endif
s = rb_str_new2(attrs->value);
#if HAVE_RB_ENC_ASSOCIATE
if (0 != pi->options->rb_enc) {
rb_enc_associate(s, pi->options->rb_enc);
}
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
}
#endif
#endif
rb_hash_aset(ah, sym, s);
}
Expand Down Expand Up @@ -343,8 +455,24 @@ end_element(PInfo pi, const char *ename) {
static void
add_instruct(PInfo pi, const char *name, Attr attrs, const char *content) {
VALUE inst;
VALUE s = rb_str_new2(name);
VALUE c = Qnil;
VALUE s;
VALUE c;
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
if (Yes == pi->options->intern_strings) {
if (0 != pi->options->rb_enc && 0 != content) {
s = rb_enc_interned_str_cstr(name, pi->options->rb_enc);
c = rb_enc_interned_str_cstr(content, pi->options->rb_enc);
} else if (0 != content) {
s = rb_interned_str_cstr(name);
c = rb_interned_str_cstr(content);
} else {
s = rb_interned_str_cstr(name);
c = Qnil;
}
} else {
c = Qnil;
#endif
s = rb_str_new2(name);

if (0 != content) {
c = rb_str_new2(content);
Expand All @@ -356,6 +484,9 @@ add_instruct(PInfo pi, const char *name, Attr attrs, const char *content) {
rb_enc_associate(c, pi->options->rb_enc);
}
}
#endif
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
}
#endif
inst = rb_obj_alloc(ox_instruct_clas);
rb_ivar_set(inst, ox_at_value_id, s);
Expand All @@ -374,9 +505,13 @@ add_instruct(PInfo pi, const char *name, Attr attrs, const char *content) {
if (Qundef == (sym = ox_cache_get(ox_symbol_cache, attrs->name, &slot, 0))) {
#if HAVE_RB_ENC_ASSOCIATE
if (0 != pi->options->rb_enc) {
#if HAVE_RB_ENC_INTERNED_STR_CSTR
VALUE rstr = rb_enc_interned_str_cstr(attrs->name, pi->options->rb_enc);
#else
VALUE rstr = rb_str_new2(attrs->name);

rb_enc_associate(rstr, pi->options->rb_enc);
#endif
sym = rb_funcall(rstr, ox_to_sym_id, 0);
} else {
sym = ID2SYM(rb_intern(attrs->name));
Expand All @@ -390,18 +525,42 @@ add_instruct(PInfo pi, const char *name, Attr attrs, const char *content) {
*slot = sym;
}
} else {
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
if (Yes == pi->options->intern_strings) {
if (0 != pi->options->rb_enc) {
sym = rb_enc_interned_str_cstr(attrs->name, pi->options->rb_enc);
} else {
sym = rb_interned_str_cstr(attrs->name);
}
} else {
#endif
sym = rb_str_new2(attrs->name);
#if HAVE_RB_ENC_ASSOCIATE
if (0 != pi->options->rb_enc) {
rb_enc_associate(sym, pi->options->rb_enc);
}
#endif
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
}
#endif
}
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
if (Yes == pi->options->intern_strings) {
if (0 != pi->options->rb_enc) {
s = rb_enc_interned_str_cstr(attrs->value, pi->options->rb_enc);
} else {
s = rb_interned_str_cstr(attrs->value);
}
} else {
#endif
s = rb_str_new2(attrs->value);
#if HAVE_RB_ENC_ASSOCIATE
if (0 != pi->options->rb_enc) {
rb_enc_associate(s, pi->options->rb_enc);
}
#endif
#if HAVE_RB_ENC_INTERNED_STR_CSTR && HAVE_RB_INTERNED_STR_CSTR
}
#endif
rb_hash_aset(ah, sym, s);
}
Expand Down

0 comments on commit b0d9a8e

Please sign in to comment.