Feature #10030: [PATCH] reduce rb_iseq_struct to 296 bytes

• Status: Closed
• Assignee: ko1 (Koichi Sasada)
• Target version: 2.2.0

[normalperson (Eric Wong)]

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

[ko1 (Koichi Sasada)]

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++) {..}

[normalperson (Eric Wong)]

[e-mail] 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++) {
	...
    }

[normalperson (Eric Wong)]

Eric Wong [e-mail] wrote:

How about using a iseq_catch_table_size macro instead:

Or static inline function (also for iseq_catch_table_bytes).

[normalperson (Eric Wong)]

• File iseq_catch_table_flex_v2.patch iseq_catch_table_flex_v2.patch added

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

[ko1 (Koichi Sasada)]

Eric Wong wrote:

Eric Wong [e-mail] 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.

[normalperson (Eric Wong)]

• File iseq_catch_table_flex_v3.patch iseq_catch_table_flex_v3.patch added

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.

[ko1 (Koichi Sasada)]

Thank you.
I will modify style after your commit.

[normalperson (Eric Wong)]

[e-mail] 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).

[ko1 (Koichi Sasada)]

(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

[ko1 (Koichi Sasada)]

• Status changed from Open to Closed