| Age | Commit message (Collapse) | Author |
|---|
|
|
|
|
|
|
|
When passing the keyword splat to [], it cannot be mutable, because
mutating the keyword splat inside [] would result in changes to the
keyword splat passed to []=.
|
|
Examples of such calls:
```ruby
obj[kw: 1] += fo
obj[**kw] &&= bar
```
Before this patch, literal keywords would segfault in the compiler,
and keyword splat usage would result in TypeError.
This handles all cases I can think of:
* literal keywords
* keyword splats
* combined with positional arguments
* combined with regular splats
* both with and without blocks
* both popped and non-popped cases
This also makes sure that to_hash is only called once on the keyword
splat argument, instead of twice, and make sure it is called before
calling to_proc on a passed block.
Fixes [Bug #20051]
Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org>
|
|
Similar as previous commit, but handles the super case with
explicit arguments.
|
|
Previously, block.to_proc was called first, by vm_caller_setup_arg_block.
kw.to_hash was called later inside CALLER_SETUP_ARG or setup_parameters_complex.
This adds a splatkw instruction that is inserted before sends with
ARGS_BLOCKARG and KW_SPLAT and without KW_SPLAT_MUT. This is not needed in the
KW_SPLAT_MUT case, because then you know the value is a hash, and you don't
need to call to_hash on it.
The splatkw instruction checks whether the second to top block is a hash,
and if not, replaces it with the value of calling to_hash on it (using
rb_to_hash_type). As it is always before a send with ARGS_BLOCKARG and
KW_SPLAT, second to top is the keyword splat, and top is the passed block.
|
|
For the following:
```
def f(*a); a end
p f(*a, kw: 3)
```
`setup_parameters_complex` pushes `{kw: 3}` onto `a`. This worked fine
back when `concatarray true` was used and `a` was already a copy. It
does not work correctly with the optimization to switch to
`concatarray false`. This duplicates the array on the callee side
in such a case.
This affects cases when passing a regular splat and a keyword splat
(or literal keywords) in a method call, where the method does not
accept keywords.
This allocation could probably be avoided, but doing so would
make `setup_parameters_complex` more complicated.
|
|
Commit e87d0882910001ef3b0c2ccd43bf00cee8c34a0c introduced a
regression where the keyword splat object passed by the caller
would be directly used by callee as keyword splat parameters,
if it implemented #to_hash. The return value of #to_hash would be
ignored in this case.
|
|
|
|
Currently, bmethod arguments are copied from the VM stack to the
C stack in vm_call_bmethod, then copied from the C stack to the VM
stack later in invoke_iseq_block_from_c. This is inefficient.
This adds vm_call_iseq_bmethod and vm_call_noniseq_bmethod.
vm_call_iseq_bmethod is an optimized method that skips stack
copies (though there is one copy to remove the receiver from
the stack), and avoids calling vm_call_bmethod_body,
rb_vm_invoke_bmethod, invoke_block_from_c_proc,
invoke_iseq_block_from_c, and vm_yield_setup_args.
Th vm_call_iseq_bmethod argument handling is similar to the
way normal iseq methods are called, and allows for similar
performance optimizations when using splats or keywords.
However, even in the no argument case it's still significantly
faster.
A benchmark is added for bmethod calling. In my environment,
it improves bmethod calling performance by 38-59% for simple
bmethod calls, and up to 180% for bmethod calls passing
literal keywords on both sides.
```
./miniruby-iseq-bmethod: 18159792.6 i/s
./miniruby-m: 13174419.1 i/s - 1.38x slower
bmethod_simple_1
./miniruby-iseq-bmethod: 15890745.4 i/s
./miniruby-m: 10008972.7 i/s - 1.59x slower
bmethod_simple_0_splat
./miniruby-iseq-bmethod: 13142804.3 i/s
./miniruby-m: 11168595.2 i/s - 1.18x slower
bmethod_simple_1_splat
./miniruby-iseq-bmethod: 12375791.0 i/s
./miniruby-m: 8491140.1 i/s - 1.46x slower
bmethod_no_splat
./miniruby-iseq-bmethod: 10151258.8 i/s
./miniruby-m: 8716664.1 i/s - 1.16x slower
bmethod_0_splat
./miniruby-iseq-bmethod: 8138802.5 i/s
./miniruby-m: 7515600.2 i/s - 1.08x slower
bmethod_1_splat
./miniruby-iseq-bmethod: 8028372.7 i/s
./miniruby-m: 5947658.6 i/s - 1.35x slower
bmethod_10_splat
./miniruby-iseq-bmethod: 6953514.1 i/s
./miniruby-m: 4840132.9 i/s - 1.44x slower
bmethod_100_splat
./miniruby-iseq-bmethod: 5287288.4 i/s
./miniruby-m: 2243218.4 i/s - 2.36x slower
bmethod_kw
./miniruby-iseq-bmethod: 8931358.2 i/s
./miniruby-m: 3185818.6 i/s - 2.80x slower
bmethod_no_kw
./miniruby-iseq-bmethod: 12281287.4 i/s
./miniruby-m: 10041727.9 i/s - 1.22x slower
bmethod_kw_splat
./miniruby-iseq-bmethod: 5618956.8 i/s
./miniruby-m: 3657549.5 i/s - 1.54x slower
```
Notes:
Merged: https://github.com/ruby/ruby/pull/7522
|
|
SystemStackError
Originally, when 2e7bceb34ea858649e1f975a934ce1894d1f06a6 fixed cfuncs to no
longer use the VM stack for large array splats, it was thought to have fully
fixed Bug #4040, since the issue was fixed for methods defined in Ruby (iseqs)
back in Ruby 2.2.
After additional research, I determined that same issue affects almost all
types of method calls, not just iseq and cfunc calls. There were two main
types of remaining issues, important cases (where large array splat should
work) and pedantic cases (where large array splat raised SystemStackError
instead of ArgumentError).
Important cases:
```ruby
define_method(:a){|*a|}
a(*1380888.times)
def b(*a); end
send(:b, *1380888.times)
:b.to_proc.call(self, *1380888.times)
def d; yield(*1380888.times) end
d(&method(:b))
def self.method_missing(*a); end
not_a_method(*1380888.times)
```
Pedantic cases:
```ruby
def a; end
a(*1380888.times)
def b(_); end
b(*1380888.times)
def c(_=nil); end
c(*1380888.times)
c = Class.new do
attr_accessor :a
alias b a=
end.new
c.a(*1380888.times)
c.b(*1380888.times)
c = Struct.new(:a) do
alias b a=
end.new
c.a(*1380888.times)
c.b(*1380888.times)
```
This patch fixes all usage of CALLER_SETUP_ARG with splatting a large
number of arguments, and required similar fixes to use a temporary
hidden array in three other cases where the VM would use the VM stack
for handling a large number of arguments. However, it is possible
there may be additional cases where splatting a large number
of arguments still causes a SystemStackError.
This has a measurable performance impact, as it requires additional
checks for a large number of arguments in many additional cases.
This change is fairly invasive, as there were many different VM
functions that needed to be modified to support this. To avoid
too much API change, I modified struct rb_calling_info to add a
heap_argv member for storing the array, so I would not have to
thread it through many functions. This struct is always stack
allocated, which helps ensure sure GC doesn't collect it early.
Because of how invasive the changes are, and how rarely large
arrays are actually splatted in Ruby code, the existing test/spec
suites are not great at testing for correct behavior. To try to
find and fix all issues, I tested this in CI with
VM_ARGC_STACK_MAX to -1, ensuring that a temporary array is used
for all array splat method calls. This was very helpful in
finding breaking cases, especially ones involving flagged keyword
hashes.
Fixes [Bug #4040]
Co-authored-by: Jimmy Miller <jimmy.miller@shopify.com>
Notes:
Merged: https://github.com/ruby/ruby/pull/7522
|
|
On the cfunc methods, if a splat argument is given, all array elements
are expanded on the VM stack and it can cause SystemStackError.
The idea to avoid it is making a hidden array to contain all parameters
and use this array as an argv.
This patch is reviesed version of https://github.com/ruby/ruby/pull/6816
The main change is all changes are closed around calling cfunc logic.
Fixes [Bug #4040]
Co-authored-by: Jeremy Evans <code@jeremyevans.net>
Notes:
Merged: https://github.com/ruby/ruby/pull/7109
|
|
As of fbaac837cfba23a9d34dc7ee144d7940248222a2, when we were performing
a safe call (`o&.x=`) with a conditional assign (`||= 1`) and discarding
the result the stack would end up in a bad state due to a missing pop.
This commit fixes that by adjusting the target label of the branchnil to
be before a pop in that case (as was previously done in the
non-conditional assignment case).
Notes:
Merged: https://github.com/ruby/ruby/pull/6437
|
|
This makes:
```ruby
args = [1, 2, -> {}]; foo(*args, &args.pop)
```
call `foo` with 1, 2, and the lambda, in addition to passing the
lambda as a block. This is different from the previous behavior,
which passed the lambda as a block but not as a regular argument,
which goes against the expected left-to-right evaluation order.
This is how Ruby already compiled arguments if using leading
arguments, trailing arguments, or keywords in the same call.
This works by disabling the optimization that skipped duplicating
the array during the splat (splatarray instruction argument
switches from false to true). In the above example, the splat
call duplicates the array. I've tested and cases where a
local variable or symbol are used do not duplicate the array,
so I don't expect this to decrease the performance of most Ruby
programs. However, programs such as:
```ruby
foo(*args, &bar)
```
could see a decrease in performance, if `bar` is a method call
and not a local variable.
This is not a perfect solution, there are ways to get around
this:
```ruby
args = Struct.new(:a).new([:x, :y])
def args.to_a; a; end
def args.to_proc; a.pop; ->{}; end
foo(*args, &args)
# calls foo with 1 argument (:x)
# not 2 arguments (:x and :y)
```
A perfect solution would require completely disabling the
optimization.
Fixes [Bug #16504]
Fixes [Bug #16500]
Notes:
Merged: https://github.com/ruby/ruby/pull/3157
|
|
* test/ruby/test_call.rb (test_safe_call): rhs should not be
evaluated when the receiver is nil. simplified the assertion
for [Bug #13964].
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@60100 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
* compile.c (iseq_compile_each0): fix stack consitency error on
attr-assign with safe navigation operator when the receiver is
nil, should pop it too. [ruby-core:83078] [Bug #13964]
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@60099 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
* compile.c (setup_args): duplicate splatting array if more
arguments present to obey left-to-right execution order.
[ruby-core:77701] [Bug# 12860]
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@56469 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
* symbol.h (is_attrset_id): ASET is an attrset ID. fix
unexpected safe call instead of an ordinary ASET.
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@53485 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
When you change this to true, you may need to add more tests.
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@53141 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
* parse.y (block_command, block_call): fix `&.` calls after
block_call. [Feature #11537]
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@53138 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
* defs/id.def (token_ops), parse.y (parser_yylex): change DOTQ
from ".?" to "&.". [ruby-core:71363] [Feature #11537]
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@52462 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
* parse.y (lbracket): remove .? before aref. [Feature #11537]
revert r52422 and r52424
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@52430 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
* parse.y (lbracket): support .? before aref. [Feature #11537]
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@52422 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
[fix GH-1066]
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@52383 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
* test_call.rb (test_safe_call): Add test cases for safe
navigation operator assignment. [Fix GH-1064]
Validate:
* can assign an attribute which is `nil`
* can "or assign" an attribute which is `nil`
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@52238 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
* compile.c (iseq_compile_each): support safe navigation of simple
attribute assignment. [Feature #11537]
* parse.y (mlhs_node, lhs, attrset_gen): ditto. keep mid
non-attrset as the sign of safe navigation.
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@52234 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
* compile.c (iseq_peephole_optimize): peephole optimization for
branchnil jumps.
* compile.c (iseq_compile_each): generate save navigation operator
code.
* insns.def (branchnil): new opcode to pop the tos and branch if
it is nil.
* parse.y (NEW_QCALL, call_op, parser_yylex): parse token '.?'.
[Feature #11537]
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@52214 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
* vm_insnhelper.c (vm_callee_setup_arg): disable fastpath if splat
argument, since argc may differ for each calls.
[ruby-core:61422] [Bug #9622]
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@45321 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@19536 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@5764 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
array as argument.
* test/ruby/test_*.rb: moved invariants to left side in
assert_equal, and use assert_nil, assert_raises and so on.
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@4516 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@4512 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
|
* test/ruby/test_*.rb: split sample/test.rb into 28 test/unit testcases. some
tests could not be translates... search '!!' mark to see it.
* test/csv/test_csv.rb: should require 'csv', not '../lib/csv'. test runner
should set load path correctly.
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@4498 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
|
