Project

General

Profile

Actions

Feature #10030

closed

[PATCH] reduce rb_iseq_struct to 296 bytes

Added by normalperson (Eric Wong) over 9 years ago. Updated over 9 years ago.

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

Description

This probably breaks ruby2cext; but I'm not sure if anybody uses that.

May we remove rb_iseq_build_for_ruby2cext?

I will probably have more patches along these lines to reduce iseq-related
allocations. This is probably the most obvious, aside from ruby2cext
compatibility.

Most iseq do not have a catch_table, so avoid needlessly adding
4-8 bytes to the struct for the common case.

  • iseq.h (struct iseq_catch_table): new flexible array struct
    (iseq_catch_table_each): new iterator macro
    (iseq_catch_table_bytes): new size macro
  • vm_core.h (struct rb_iseq_struct): use _catch_table member
  • compile.c (iseq_set_exception_table): update for struct changes
  • iseq.c (iseq_free): ditto
  • iseq.c (iseq_memsize): ditto
  • iseq.c (rb_iseq_disasm): ditto
  • iseq.c (iseq_data_to_ary): ditto
  • iseq.c (rb_iseq_build_for_ruby2cext): ditto (untested)
  • vm.c (vm_exec): ditto
  • vm_core.h (struct rb_iseq_struct): ditto
  • vm_insnhelper.c (vm_throw): ditto

Files

iseq_catch_table_flex.patch (8.94 KB) iseq_catch_table_flex.patch normalperson (Eric Wong), 07/13/2014 07:09 AM
iseq_catch_table_flex_v2.patch (8.72 KB) iseq_catch_table_flex_v2.patch normalperson (Eric Wong), 07/14/2014 01:20 AM
iseq_catch_table_flex_v3.patch (9.05 KB) iseq_catch_table_flex_v3.patch normalperson (Eric Wong), 07/14/2014 06:10 AM

Updated by ko1 (Koichi Sasada) over 9 years ago

From what size on which architecture?

On the code:

  1. _catch_table should be catch_table
  2. I don't like iseq_catch_table_each() macro.
    I like to write like
  if (iseq->_catch_table) for (i=0; i<iseq->_catch_table->size; i++) {..}

Updated by normalperson (Eric Wong) over 9 years ago

wrote:

From what size on which architecture?

x86-64

On the code:

(1) _catch_table should be catch_table

OK, I will change it back. I renamed it so it'd be easier for the
compiler to detect places where I need to change code.

(2) I don't like iseq_catch_table_each() macro.
I like to write like

  if (iseq->_catch_table) for (i=0; i<iseq->_catch_table->size; i++) {..}

OK, I didn't want to increase line length or change indentation of
existing code.

How about using a iseq_catch_table_size macro instead:

#define iseq_catch_table_size(iseq) \
    (iseq->catch_table ? iseq->catch_table->size : 0)

    for (i = 0; i < iseq_catch_table_size(iseq); i++) {
	...
    }

Updated by normalperson (Eric Wong) over 9 years ago

Eric Wong wrote:

How about using a iseq_catch_table_size macro instead:

Or static inline function (also for iseq_catch_table_bytes).

Updated by normalperson (Eric Wong) over 9 years ago

Updated patch

Changes from v1:

  • renamed iseq->_catch_table to iseq->catch_table
  • iseq_catch_table_bytes: made a static inline function
  • iseq_catch_table_size: new function replaces the
    iseq_catch_table_each iterator macro

Updated by ko1 (Koichi Sasada) over 9 years ago

Eric Wong wrote:

Eric Wong wrote:

How about using a iseq_catch_table_size macro instead:

Or static inline function (also for iseq_catch_table_bytes).

There are two reasons I'm opposed to:

(1) I think writing "if (...) for (...)" is more clear (easy to understand).
I don't care to adding one line to check exisiting a catch table.

(2) Maybe it is difficult to eliminate the check at eac iteration by detecting loop invariant.

Of course, (2) is very trivial (small impact), but with (1), I like this approach.

Updated by normalperson (Eric Wong) over 9 years ago

Changes from v2:

  • iseq_catch_table_size removed, use if (...) for (;...;)

I used a short local variable in some places to keep the "if (...) for (...)"
sequences from wrapping on long lines.

Updated by ko1 (Koichi Sasada) over 9 years ago

Thank you.
I will modify style after your commit.

Updated by normalperson (Eric Wong) over 9 years ago

wrote:

Thank you.
I will modify style after your commit.

OK, r46811 (I also made trivial commit r46813 to shrink catch entry)

What should we do about rb_iseq_build_for_ruby2cext? I think it is
broken, now (as is anything else which might depend on iseq internals).

Updated by ko1 (Koichi Sasada) over 9 years ago

(2014/07/14 16:12), Eric Wong wrote:

OK, r46811 (I also made trivial commit r46813 to shrink catch entry)

Thanks.

What should we do about rb_iseq_build_for_ruby2cext? I think it is
broken, now (as is anything else which might depend on iseq internals).

It is only for experimental feature.

--
// SASADA Koichi at atdot dot net

Updated by ko1 (Koichi Sasada) over 9 years ago

  • Status changed from Open to Closed
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0