Feature #10030

[PATCH] reduce rb_iseq_struct to 296 bytes

Added by Eric Wong about 1 year ago. Updated about 1 year ago.

[ruby-core:63681]
Status:Closed
Priority:Normal
Assignee:Koichi Sasada

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

iseq_catch_table_flex.patch Magnifier (8.94 KB) Eric Wong, 07/13/2014 07:09 AM

iseq_catch_table_flex_v2.patch Magnifier (8.72 KB) Eric Wong, 07/14/2014 01:20 AM

iseq_catch_table_flex_v3.patch Magnifier (9.05 KB) Eric Wong, 07/14/2014 06:10 AM

History

#1 Updated by Koichi Sasada about 1 year 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++) {..}

#2 Updated by Eric Wong about 1 year ago

ko1@atdot.net 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++) {
    ...
     }

#3 Updated by Eric Wong about 1 year ago

Eric Wong normalperson@yhbt.net wrote:

How about using a iseq_catch_table_size macro instead:

Or static inline function (also for iseq_catch_table_bytes).

#4 Updated by Eric Wong about 1 year 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

#5 Updated by Koichi Sasada about 1 year ago

Eric Wong wrote:

Eric Wong normalperson@yhbt.net 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.

#6 Updated by Eric Wong about 1 year 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.

#7 Updated by Koichi Sasada about 1 year ago

Thank you.
I will modify style after your commit.

#8 Updated by Eric Wong about 1 year ago

ko1@atdot.net 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).

#9 Updated by Koichi Sasada about 1 year 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

#10 Updated by Koichi Sasada about 1 year ago

  • Status changed from Open to Closed

Also available in: Atom PDF