Feature #11664: [PATCH] introduce rb_autoload_value to replace rb_autoload

• Status: Closed
• Assignee: -
• Target version: -

[normalperson (Eric Wong)]

rb_autoload_value may be safer by preventing premature GC. It
can also be more efficient by passing a pre-frozen string that
can be deduped using rb_fstring. Common autoload callers (e.g.
rubygems, rdoc) already use string literals as the file
argument.

There seems to be no reason to expose rb_autoload_value to the
public C API since autoload is not performance-critical.
Applications may declare autoloads in Ruby code or via
rb_funcall; so merely deprecate rb_autoload without exposing
rb_autoload_value to new users.

Running: valgrind -v ruby -rrdoc -rubygems -e exit
shows a minor memory reduction (32-bit userspace)

before:

  in use at exit: 1,600,621 bytes in 28,819 blocks
total heap usage: 55,786 allocs, 26,967 frees, 6,693,790 bytes allocated

after:

  in use at exit: 1,599,778 bytes in 28,789 blocks
total heap usage: 55,739 allocs, 26,950 frees, 6,692,973 bytes allocated

[normalperson (Eric Wong)]

[e-mail] wrote:

https://bugs.ruby-lang.org/issues/11664

Comments? It's low-risk, but does deprecate a (probably unused)
public C-API.

Thanks.

[nobu (Nobuyoshi Nakada)]

• Description updated (diff)

The new function looks fine to me, but why deprecating rb_autoload?

We used to append _str in many cases, IIRC.

[normalperson (Eric Wong)]

[e-mail] wrote:

The new function looks fine to me, but why deprecating rb_autoload?

In general, I prefer to make the C API smaller so it is easier to
support and improve Ruby internals. The rb_autoload API is also
tricky/dangerous with RSTRING_PTR use because of GC.

Since declaring autoload is not performance-critical, users who
need autoload from C-ext can use rb_funcall instead.

We used to append _str in many cases, IIRC.

So rename rb_autoload_value => rb_autoload_str?

[normalperson (Eric Wong)]

Eric Wong [e-mail] wrote:

[e-mail] wrote:

We used to append _str in many cases, IIRC.

So rename rb_autoload_value => rb_autoload_str?

Did that, I'd like to commit the following, soon:

http://80x24.org/spew/20151204004808.3288-1-e%4080x24.org/raw

• Status changed from Open to Closed

Applied in changeset r52909.

---

introduce rb_autoload_str to replace rb_autoload

rb_autoload_str may be safer by preventing premature GC. It
can also be more efficient by passing a pre-frozen string that
can be deduped using rb_fstring. Common autoload callers (e.g.
rubygems, rdoc) already use string literals as the file
argument.

There seems to be no reason to expose rb_autoload_str to the
public C API since autoload is not performance-critical.
Applications may declare autoloads in Ruby code or via
rb_funcall; so merely deprecate rb_autoload without exposing
rb_autoload_str to new users.

Running: valgrind -v ruby -rrdoc -rubygems -e exit
shows a minor memory reduction (32-bit userspace)

before:

in use at exit: 1,600,621 bytes in 28,819 blocks
total heap usage: 55,786 allocs, 26,967 frees, 6,693,790 bytes allocated

after:

in use at exit: 1,599,778 bytes in 28,789 blocks
total heap usage: 55,739 allocs, 26,950 frees, 6,692,973 bytes allocated

• include/ruby/intern.h (rb_autoload): deprecate
• internal.h (rb_autoload_str): declare
• load.c (rb_mod_autoload): use rb_autoload_str
• variable.c (rb_autoload): become compatibility wrapper
  (rb_autoload_str): hoisted out from old rb_autoload
  [ruby-core:71369] [Feature #11664]

[nobu (Nobuyoshi Nakada)]

I don't think there is good enough reason to deprecate rb_autoload.

[normalperson (Eric Wong)]

[e-mail] wrote:

I don't think there is good enough reason to deprecate rb_autoload.

Who uses rb_autoload from C extensions?
It is not performance-critical.

I'd like to make the C API smaller so we can have less to support
in the future. Deprecating unused, non-performance-critical APIs
won't hurt anybody; and hopefully we'll be able to remove the
old function in a few years.

If you want to un-deprecate, go ahead. I just don't see the point
of maintaining the old function forever.

(and maybe autoload will be removed entirely by matz)