From: "Lourens Naudé" Date: 2020-01-27T06:02:49+00:00 Subject: [ruby-core:97002] Re: [Ruby master Feature#16562] Expose rb_io_set_encoding_internal to reduce function calls on loading source files --===============0857955863== Content-Type: multipart/alternative; boundary="00000000000016aa43059d18dc47" --00000000000016aa43059d18dc47 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Good questions aren=E2=80=99t nitpicks :-) References this comment https://github.com/ruby/ruby/pull/2829#discussion_r365477407 , but I may have interpreted it too literal ... On Mon, 27 Jan 2020 at 01:15, wrote: > Issue #16562 has been updated by shyouhei (Shyouhei Urabe). > > > This might be a nitpick but, can you tell us the reason why > rb_io_set_encoding_internal has to be exposed (that is, callable from 3rd > party extension libraries)? I understand it needs to be non-`static` but > it seems not need to be truly globally visible. I guess I missed somethi= ng. > > ---------------------------------------- > Feature #16562: Expose rb_io_set_encoding_internal to reduce function > calls on loading source files > https://bugs.ruby-lang.org/issues/16562#change-84058 > > * Author: methodmissing (Lourens Naud=C3=A9) > * Status: Open > * Priority: Normal > * Assignee: > * Target version: > ---------------------------------------- > References https://github.com/ruby/ruby/pull/2829 > > Removes a few `rb_funcall` calls from source file reading and the AST > loader. > > * `rb_ast_parse_file`: removes 1 x `rb_funcall`, removes 1 transient > string allocation (`"-"`) > * `load_file_internal`: removes 3 x `rb_funcall`, removes 1 transient > string allocation (`"-"`) > * Removes a branch from `rb_io_set_encoding` which checks for a T_FILE > type which I could not take with ruby tests or Rails (asserted with an > inline `rb_bug` for backtrace :smile: ). I declare it dead > * Removes static symbol `id_set_encoding` from `io.c` > * Introduces a literal fstring `str_no_transcoding` to represent the no > transcoding option `"-"` > > Now ... I don't think I like the API naming convention of > `rb_io_set_encoding_internal` very much for the following reason: with > `internal` it means not public API (but then again declaring it in > `ruby/io.h` implies that too, but can also be interpreted as `internal > encoding`, which can be confusing. > > Open to ideas about the API and also the special `Qfalse` argument which > assigns the no transcoding special case. > > Master booting Redmine with Rails 6: > > ``` > [RUBY_DEBUG_COUNTER] obj_newobj 2095742 > [RUBY_DEBUG_COUNTER] frame_push_cfunc 2217390 > ``` > > This PR: > > ``` > [RUBY_DEBUG_COUNTER] obj_newobj 2093369 > [RUBY_DEBUG_COUNTER] frame_push_cfunc 2212686 > ``` > > Diffs: > > * `4704` less C function calls (my understanding is `rb_call` and friends > would bump `frame_push_cfunc`) > * `2373` less transient objects allocated (about half of the dispatch > calls also allocated a "-" string) > > Which is about inline with the loaded features for booting the process: > > ``` > irb(main):002:0> $LOADED_FEATURES.size > =3D> 2165 > ``` > > > > -- > https://bugs.ruby-lang.org/ > > Unsubscribe: > > --00000000000016aa43059d18dc47 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Good questions aren=E2=80=99t nitpicks :-)

References this comment=C2= =A0
https://github.com/ruby/ruby/pull/2829#discussion_r365477407 , bu= t I may have interpreted it too literal ...
On Mon, 27 Jan 2020 at 01:15, <shyouhei@ruby-lang.org> wrote:
Issue #16562 has been updated by shyouhei (Shyouhei Urabe).


This might be a nitpick but, can you tell us the reason why rb_io_set_encod= ing_internal has to be exposed (that is, callable from 3rd party extension = libraries)?=C2=A0 I understand it needs to be non-`static` but it seems not= need to be truly globally visible.=C2=A0 I guess I missed something.

----------------------------------------
Feature #16562: Expose rb_io_set_encoding_internal to reduce function calls= on loading source files
https://bugs.ruby-lang.org/issues/16562#change-8= 4058

* Author: methodmissing (Lourens Naud=C3=A9)
* Status: Open
* Priority: Normal
* Assignee:
* Target version:
----------------------------------------
References https://github.com/ruby/ruby/pull/2829

Removes a few `rb_funcall` calls from source file reading and the AST loade= r.

* `rb_ast_parse_file`: removes 1 x `rb_funcall`, removes 1 transient string= allocation (`"-"`)
* `load_file_internal`: removes 3 x `rb_funcall`, removes 1 transient strin= g allocation (`"-"`)
* Removes a branch from `rb_io_set_encoding` which checks for a T_FILE type= which I could not take with ruby tests or Rails (asserted with an inline `= rb_bug` for backtrace :smile: ). I declare it dead
* Removes static symbol `id_set_encoding` from `io.c`
* Introduces a literal fstring `str_no_transcoding` to represent the no tra= nscoding option `"-"`

Now ... I don't think I like the API naming convention of `rb_io_set_en= coding_internal` very much for the following reason: with `internal` it mea= ns not public API (but then again declaring it in `ruby/io.h` implies that = too, but can also be interpreted as `internal encoding`, which can be confu= sing.

Open to ideas about the API and also the special `Qfalse` argument which as= signs the no transcoding special case.

Master booting Redmine with Rails 6:

```
[RUBY_DEBUG_COUNTER]=C2=A0 =C2=A0 obj_newobj=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A020= 95742
[RUBY_DEBUG_COUNTER]=C2=A0 =C2=A0 frame_push_cfunc=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A02217390
```

This PR:

```
[RUBY_DEBUG_COUNTER]=C2=A0 =C2=A0 obj_newobj=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A020= 93369
[RUBY_DEBUG_COUNTER]=C2=A0 =C2=A0 frame_push_cfunc=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A02212686
```

Diffs:

* `4704` less C function calls (my understanding is `rb_call` and friends w= ould bump `frame_push_cfunc`)
* `2373` less transient objects allocated (about half of the dispatch calls= also allocated a "-" string)

Which is about inline with the loaded features for booting the process:

```
irb(main):002:0> $LOADED_FEATURES.size
=3D> 2165
```



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

Unsubscribe: <mailto:ruby-core-request@ruby-lang.org?subject=3Dunsubscribe= >
<http://lists.ruby-lang.org/cgi-bin/m= ailman/options/ruby-core>
--00000000000016aa43059d18dc47-- --===============0857955863== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline Unsubscribe: --===============0857955863==--