Skip to content

Commit

Permalink
Make defined? cache the results of method calls
Browse files Browse the repository at this point in the history 
Previously, defined? could result in many more method calls than
the code it was checking. `defined? a.b.c.d.e.f` generated 15 calls,
with `a` called 5 times, `b` called 4 times, etc..  This was due to
the fact that defined works in a recursive manner, but it previously
did not cache results.  So for `defined? a.b.c.d.e.f`, the logic was
similar to

```ruby
return nil unless defined? a
return nil unless defined? a.b
return nil unless defined? a.b.c
return nil unless defined? a.b.c.d
return nil unless defined? a.b.c.d.e
return nil unless defined? a.b.c.d.e.f
"method"
```

With this change, the logic is similar to the following, without
the creation of a local variable:

```ruby
return nil unless defined? a
_ = a
return nil unless defined? _.b
_ = _.b
return nil unless defined? _.c
_ = _.c
return nil unless defined? _.d
_ = _.d
return nil unless defined? _.e
_ = _.e
return nil unless defined? _.f
"method"
```

In addition to eliminating redundant method calls for defined
statements, this greatly simplifies the instruction sequences by
eliminating duplication.  Previously:

```
0000 putnil                                                           (   1)[Li]
0001 putself
0002 defined                                func, :a, false
0006 branchunless                           73
0008 putself
0009 opt_send_without_block                 <calldata!mid:a, argc:0, FCALL|VCALL|ARGS_SIMPLE>
0011 defined                                method, :b, false
0015 branchunless                           73
0017 putself
0018 opt_send_without_block                 <calldata!mid:a, argc:0, FCALL|VCALL|ARGS_SIMPLE>
0020 opt_send_without_block                 <calldata!mid:b, argc:0, ARGS_SIMPLE>
0022 defined                                method, :c, false
0026 branchunless                           73
0028 putself
0029 opt_send_without_block                 <calldata!mid:a, argc:0, FCALL|VCALL|ARGS_SIMPLE>
0031 opt_send_without_block                 <calldata!mid:b, argc:0, ARGS_SIMPLE>
0033 opt_send_without_block                 <calldata!mid:c, argc:0, ARGS_SIMPLE>
0035 defined                                method, :d, false
0039 branchunless                           73
0041 putself
0042 opt_send_without_block                 <calldata!mid:a, argc:0, FCALL|VCALL|ARGS_SIMPLE>
0044 opt_send_without_block                 <calldata!mid:b, argc:0, ARGS_SIMPLE>
0046 opt_send_without_block                 <calldata!mid:c, argc:0, ARGS_SIMPLE>
0048 opt_send_without_block                 <calldata!mid:d, argc:0, ARGS_SIMPLE>
0050 defined                                method, :e, false
0054 branchunless                           73
0056 putself
0057 opt_send_without_block                 <calldata!mid:a, argc:0, FCALL|VCALL|ARGS_SIMPLE>
0059 opt_send_without_block                 <calldata!mid:b, argc:0, ARGS_SIMPLE>
0061 opt_send_without_block                 <calldata!mid:c, argc:0, ARGS_SIMPLE>
0063 opt_send_without_block                 <calldata!mid:d, argc:0, ARGS_SIMPLE>
0065 opt_send_without_block                 <calldata!mid:e, argc:0, ARGS_SIMPLE>
0067 defined                                method, :f, true
0071 swap
0072 pop
0073 leave
```

After change:

```
0000 putnil                                                           (   1)[Li]
0001 putself
0002 dup
0003 defined                                func, :a, false
0007 branchunless                           52
0009 opt_send_without_block                 <calldata!mid:a, argc:0, FCALL|VCALL|ARGS_SIMPLE>
0011 dup
0012 defined                                method, :b, false
0016 branchunless                           52
0018 opt_send_without_block                 <calldata!mid:b, argc:0, ARGS_SIMPLE>
0020 dup
0021 defined                                method, :c, false
0025 branchunless                           52
0027 opt_send_without_block                 <calldata!mid:c, argc:0, ARGS_SIMPLE>
0029 dup
0030 defined                                method, :d, false
0034 branchunless                           52
0036 opt_send_without_block                 <calldata!mid:d, argc:0, ARGS_SIMPLE>
0038 dup
0039 defined                                method, :e, false
0043 branchunless                           52
0045 opt_send_without_block                 <calldata!mid:e, argc:0, ARGS_SIMPLE>
0047 defined                                method, :f, true
0051 swap
0052 pop
0053 leave
```

This fixes issues where for pathological small examples, Ruby would generate
huge instruction sequences.

Unfortunately, implementing this support is kind of a hack.  This adds another
parameter to compile_call for whether we should assume the receiver is already
present on the stack, and has defined? set that parameter for the specific
case where it is compiling a method call where the receiver is also a method
call.

defined_expr0 also takes an additional parameter for whether it should leave
the results of the method call on the stack.  If that argument is true, in
the case where the method isn't defined, we jump to the pop before the leave,
so the extra result is not left on the stack.  This requires space for an
additional label, so lfinish now needs to be able to hold 3 labels.

Fixes [Bug #17649]
Fixes [Bug #13708]
  • Loading branch information
@jeremyevans
jeremyevans committed Mar 29, 2021
1 parent 190a57b commit 7b3c5ab
Showing 1 changed file with 66 additions and 33 deletions.
99 changes: 66 additions & 33 deletions compile.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4883,9 +4883,13 @@ static void
defined_expr(rb_iseq_t *iseq, LINK_ANCHOR *const ret,
const NODE *const node, LABEL **lfinish, VALUE needstr);

static int
compile_call(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const node, const enum node_type type, int line, int popped, bool assume_receiver);

static void
defined_expr0(rb_iseq_t *iseq, LINK_ANCHOR *const ret,
const NODE *const node, LABEL **lfinish, VALUE needstr)
const NODE *const node, LABEL **lfinish, VALUE needstr,
bool keep_result)
{
enum defined_type expr_type = DEFINED_NOT_DEFINED;
enum node_type type;
Expand All @@ -4911,7 +4915,7 @@ defined_expr0(rb_iseq_t *iseq, LINK_ANCHOR *const ret,
const NODE *vals = node;

do {
defined_expr0(iseq, ret, vals->nd_head, lfinish, Qfalse);
defined_expr0(iseq, ret, vals->nd_head, lfinish, Qfalse, false);

if (!lfinish[1]) {
lfinish[1] = NEW_LABEL(line);
Expand Down Expand Up @@ -4963,7 +4967,7 @@ defined_expr0(rb_iseq_t *iseq, LINK_ANCHOR *const ret,
if (!lfinish[1]) {
lfinish[1] = NEW_LABEL(line);
}
defined_expr0(iseq, ret, node->nd_head, lfinish, Qfalse);
defined_expr0(iseq, ret, node->nd_head, lfinish, Qfalse, false);
ADD_INSNL(ret, line, branchunless, lfinish[1]);
NO_CHECK(COMPILE(ret, "defined/colon2#nd_head", node->nd_head));

Expand Down Expand Up @@ -4992,22 +4996,45 @@ defined_expr0(rb_iseq_t *iseq, LINK_ANCHOR *const ret,
(type == NODE_CALL || type == NODE_OPCALL ||
(type == NODE_ATTRASGN && !private_recv_p(node)));

if (!lfinish[1] && (node->nd_args || explicit_receiver)) {
lfinish[1] = NEW_LABEL(line);
}
if (node->nd_args || explicit_receiver) {
if (!lfinish[1]) {
lfinish[1] = NEW_LABEL(line);
}
if (!lfinish[2]) {
lfinish[2] = NEW_LABEL(line);
}
}
if (node->nd_args) {
defined_expr0(iseq, ret, node->nd_args, lfinish, Qfalse);
defined_expr0(iseq, ret, node->nd_args, lfinish, Qfalse, false);
ADD_INSNL(ret, line, branchunless, lfinish[1]);
}
if (explicit_receiver) {
defined_expr0(iseq, ret, node->nd_recv, lfinish, Qfalse);
ADD_INSNL(ret, line, branchunless, lfinish[1]);
NO_CHECK(COMPILE(ret, "defined/recv", node->nd_recv));
defined_expr0(iseq, ret, node->nd_recv, lfinish, Qfalse, true);
switch(nd_type(node->nd_recv)) {
case NODE_CALL:
case NODE_OPCALL:
case NODE_VCALL:
case NODE_FCALL:
case NODE_ATTRASGN:
ADD_INSNL(ret, line, branchunless, lfinish[2]);
compile_call(iseq, ret, node->nd_recv, nd_type(node->nd_recv), line, 0, true);
break;
default:
ADD_INSNL(ret, line, branchunless, lfinish[1]);
NO_CHECK(COMPILE(ret, "defined/recv", node->nd_recv));
break;
}
if (keep_result) {
ADD_INSN(ret, line, dup);
}
ADD_INSN3(ret, line, defined, INT2FIX(DEFINED_METHOD),
ID2SYM(node->nd_mid), PUSH_VAL(DEFINED_METHOD));
}
else {
ADD_INSN(ret, line, putself);
if (keep_result) {
ADD_INSN(ret, line, dup);
}
ADD_INSN3(ret, line, defined, INT2FIX(DEFINED_FUNC),
ID2SYM(node->nd_mid), PUSH_VAL(DEFINED_METHOD));
}
Expand Down Expand Up @@ -5075,7 +5102,7 @@ defined_expr(rb_iseq_t *iseq, LINK_ANCHOR *const ret,
const NODE *const node, LABEL **lfinish, VALUE needstr)
{
LINK_ELEMENT *lcur = ret->last;
defined_expr0(iseq, ret, node, lfinish, needstr);
defined_expr0(iseq, ret, node, lfinish, needstr, false);
if (lfinish[1]) {
int line = nd_line(node);
LABEL *lstart = NEW_LABEL(line);
Expand Down Expand Up @@ -5104,14 +5131,18 @@ compile_defined_expr(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const
ADD_INSN1(ret, line, putobject, str);
}
else {
LABEL *lfinish[2];
LABEL *lfinish[3];
LINK_ELEMENT *last = ret->last;
lfinish[0] = NEW_LABEL(line);
lfinish[1] = 0;
lfinish[2] = 0;
defined_expr(iseq, ret, node->nd_head, lfinish, needstr);
if (lfinish[1]) {
ELEM_INSERT_NEXT(last, &new_insn_body(iseq, line, BIN(putnil), 0)->link);
ADD_INSN(ret, line, swap);
if (lfinish[2]) {
ADD_LABEL(ret, lfinish[2]);
}
ADD_INSN(ret, line, pop);
ADD_LABEL(ret, lfinish[1]);
}
Expand Down Expand Up @@ -7416,7 +7447,7 @@ compile_builtin_function_call(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NOD
}

static int
compile_call(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const node, const enum node_type type, int line, int popped)
compile_call(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const node, const enum node_type type, int line, int popped, bool assume_receiver)
{
/* call: obj.method(...)
* fcall: func(...)
Expand Down Expand Up @@ -7508,28 +7539,30 @@ compile_call(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const node, co
}

/* receiver */
if (type == NODE_CALL || type == NODE_OPCALL || type == NODE_QCALL) {
int idx, level;
if (!assume_receiver) {
if (type == NODE_CALL || type == NODE_OPCALL || type == NODE_QCALL) {
int idx, level;

if (mid == idCall &&
nd_type(node->nd_recv) == NODE_LVAR &&
iseq_block_param_id_p(iseq, node->nd_recv->nd_vid, &idx, &level)) {
ADD_INSN2(recv, nd_line(node->nd_recv), getblockparamproxy, INT2FIX(idx + VM_ENV_DATA_SIZE - 1), INT2FIX(level));
}
else if (private_recv_p(node)) {
ADD_INSN(recv, nd_line(node), putself);
flag |= VM_CALL_FCALL;
}
else {
CHECK(COMPILE(recv, "recv", node->nd_recv));
}

if (mid == idCall &&
nd_type(node->nd_recv) == NODE_LVAR &&
iseq_block_param_id_p(iseq, node->nd_recv->nd_vid, &idx, &level)) {
ADD_INSN2(recv, nd_line(node->nd_recv), getblockparamproxy, INT2FIX(idx + VM_ENV_DATA_SIZE - 1), INT2FIX(level));
}
else if (private_recv_p(node)) {
ADD_INSN(recv, nd_line(node), putself);
flag |= VM_CALL_FCALL;
if (type == NODE_QCALL) {
else_label = qcall_branch_start(iseq, recv, &branches, node, line);
}
}
else {
CHECK(COMPILE(recv, "recv", node->nd_recv));
else if (type == NODE_FCALL || type == NODE_VCALL) {
ADD_CALL_RECEIVER(recv, line);
}

if (type == NODE_QCALL) {
else_label = qcall_branch_start(iseq, recv, &branches, node, line);
}
}
else if (type == NODE_FCALL || type == NODE_VCALL) {
ADD_CALL_RECEIVER(recv, line);
}

/* args */
Expand Down Expand Up @@ -8150,7 +8183,7 @@ iseq_compile_each0(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *node, in
case NODE_QCALL: /* obj&.foo */
case NODE_FCALL: /* foo() */
case NODE_VCALL: /* foo (variable or call) */
if (compile_call(iseq, ret, node, type, line, popped) == COMPILE_NG) {
if (compile_call(iseq, ret, node, type, line, popped, false) == COMPILE_NG) {
goto ng;
}
break;
Expand Down

0 comments on commit 7b3c5ab

Please sign in to comment.