Project

General

Profile

Actions

Feature #20011

closed

Reduce implicit array allocations on caller side of method calling

Added by jeremyevans0 (Jeremy Evans) 12 months ago. Updated 11 months ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:115428]

Description

I would like to use the peephole optimizer to eliminate caller-side array allocations for the following cases, by switching splatarray true to splatarray false:

f(1, *a)
f(1, *a, &lvar)
f(1, *a, &@ivar)
f(*a, **lvar)
f(*a, **@ivar)
f(*a, **lvar, &lvar)
f(*a, **@ivar, &@ivar)
f(*a, kw: 1)
f(*a, kw:1, &lvar)
f(*a, kw:1, &@ivar)

In terms of safety, currently, f(*a, &lvar) and f(*a, &@ivar) both avoid array allocations (splatarray false), and all of the above are as safe as those in terms of safety. Note that since at least Ruby 1.8, in pathlogical cases, lvar.to_proc can modify a, which results in behavior contrary to expected evaluation order:

ary = [1,2]
kwd = Object.new
kwd.define_singleton_method(:to_proc) {ary << 4; lambda{}}

p(*ary, &kwd)
# 4 included in output

I think that for both the current f(*a, &lvar) and f(*a, &@ivar) cases and all of the above cases, the benefit of avoiding the allocation is higher than the costs, considering that only pathologic cases fail. If we do not want the above optimization, for consistency, we should remove the optimization of f(*a, &lvar) and f(*a, &@ivar) (or update the optimization so that it is only used if lvar or @ivar is already a proc), which will slow Ruby down.

I have submitted a pull request for these changes: https://github.com/ruby/ruby/pull/8853

To make sure these cases are worth optimizing, I did some analysis:

For ruby -e '' (just loads rubygems):

5 : f(1, *a)
2 : f(1, *a, &lvar) | f(1, *a, &@ivar)

Current stdlib:

139 : f(1, *a)
3 : f(1, *a, &lvar) | f(1, *a, &@ivar)
4 : f(*a, **lvar) | f(*a, **@ivar)

Rails master:

77 : f(1, *a)
5 : f(1, *a, &lvar) | f(1, *a, &@ivar)
26 : f(*a, **lvar) | f(*a, **@ivar)
5 : f(*a, kw: 1)

minitest 5.20 (bundled gem):

4 : f(1, *a)
1 : f(1, *a, &lvar) | f(1, *a, &@ivar)
2 : f(*a, **lvar) | f(*a, **@ivar)
2 : f(*a, **lvar, &lvar) | f(*a, **@ivar, &@ivar)

capybara 3.39.2:

 15 : f(1, *a)
  2 : f(1, *a, &lvar) | f(1, *a, &@ivar)
 19 : f(*a, **lvar) | f(*a, **@ivar)
  4 : f(*a, **lvar, &lvar) | f(*a, **@ivar, &@ivar)

This shows that all cases optimized except f(*a, kw:1, &lvar) and f(*a, kw:1, &@ivar) are used in common gems. Those cases could be removed if people think they are not worth optimizing.

Code backing the above analysis can be found at https://github.com/ruby/ruby/pull/8853#issuecomment-1817656139

Updated by byroot (Jean Boussier) 12 months ago

I tried this on Shopify's monolith CI and it's breaking quite hard:

      ATTRIBUTE_NAMES = T.let(
        [
          :currency,
          :description,
          :kind,
          :price,
          :reasons,
          :receipt,
          :shipping_service_charge_id,
          :shipping_service_label_id,
          :origin_address,
        ].freeze,
        T::Array[Symbol]
      )

      T.unsafe(self).validates(*ATTRIBUTE_NAMES, presence: true) # here
 `<class:Payload>': can't modify frozen Array: [:currency, :description, :kind, :price, :reasons, :receipt, :shipping_service_charge_id, :shipping_service_label_id, :origin_address] (FrozenError)
	from /app/components/delivery/app/services/shipping/adjustment_creator/payload.rb:6:in `<class:AdjustmentCreator>'
	from /app/components/delivery/app/services/shipping/adjustment_creator/payload.rb:5:in `<module:Shipping>'
	from /app/components/delivery/app/services/shipping/adjustment_creator/payload.rb:4:in `<main>'

(I originally reported this on the wrong issue)

Updated by jeremyevans0 (Jeremy Evans) 12 months ago

byroot (Jean Boussier) wrote in #note-1:

I tried this on Shopify's monolith CI and it's breaking quite hard:

Thanks for testing. I think I found and fixed the issue: https://github.com/ruby/ruby/pull/8853/commits/d9a6f7a77ab5d682950f891990401a95f8fdc6f0

Can you test and see if that fixes it, or if there is another issue?

Updated by ko1 (Koichi Sasada) 11 months ago

Could you provide benchmarks?

Updated by jeremyevans0 (Jeremy Evans) 11 months ago

ko1 (Koichi Sasada) wrote in #note-4:

Could you provide benchmarks?

I added a benchmark to the pull request. Here are the results:

                        arg_splat
    after-patch:  11633482.8 i/s
   before-patch:   6603689.5 i/s - 1.76x  slower

                  arg_splat_block
    after-patch:  10011285.1 i/s
   before-patch:   6072490.8 i/s - 1.65x  slower

                   splat_kw_splat
    after-patch:   5574867.8 i/s
   before-patch:   4025629.4 i/s - 1.38x  slower

             splat_kw_splat_block
    after-patch:   4977728.5 i/s
   before-patch:   3827400.9 i/s - 1.30x  slower

                         splat_kw
    after-patch:   3686117.6 i/s
   before-patch:   3103409.3 i/s - 1.19x  slower

                   splat_kw_block
    after-patch:   3535774.1 i/s
   before-patch:   2957890.6 i/s - 1.20x  slower

So a 20%-75% increase in performance depending on the type of method call.

Updated by jeremyevans0 (Jeremy Evans) 11 months ago

I expanded the pull request to handle getblockparamproxy for block arguments, in addition to getlocal and getinstancevariable. getblockparamproxyis used inside methods, when the block being passed in the current calls was passed as the block to the current method. This expanded the number of cases optimized:

stdlib:

144 : f(1, *a)
  5 : f(1, *a, &lvar) | f(1, *a, &@ivar)
  4 : f(*a, **lvar) | f(*a, **@ivar)
  3 : f(*a, **lvar, &lvar) | f(*a, **@ivar, &@ivar)

Rails master lib:

 82 : f(1, *a)
  7 : f(1, *a, &lvar) | f(1, *a, &@ivar)
 26 : f(*a, **lvar) | f(*a, **@ivar)
  6 : f(*a, **lvar, &lvar) | f(*a, **@ivar, &@ivar)
  5 : f(*a, kw: 1)

minitest 5.20 (bundled gem):

  9 : f(1, *a)
  3 : f(1, *a, &lvar) | f(1, *a, &@ivar)
  2 : f(*a, **lvar) | f(*a, **@ivar)
  4 : f(*a, **lvar, &lvar) | f(*a, **@ivar, &@ivar)

capybara 3.39.2:

 20 : f(1, *a)
  4 : f(1, *a, &lvar) | f(1, *a, &@ivar)
 19 : f(*a, **lvar) | f(*a, **@ivar)
 14 : f(*a, **lvar, &lvar) | f(*a, **@ivar, &@ivar)

Updated by ko1 (Koichi Sasada) 11 months ago

Thank you. Please try it.

Actions #8

Updated by mame (Yusuke Endoh) 11 months ago

  • Status changed from Open to Closed
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0