[ruby-core:113242] [Ruby master Bug#4040] SystemStackError with Hash[*a] for Large _a_

Issue #4040 has been updated by jeremyevans0 (Jeremy Evans).


ko1 (Koichi Sasada) wrote in #note-17:
> Quote from devmeeting agenda https://bugs.ruby-lang.org/issues/19525:
> 
> > The fix results in a minor performance decrease in microbenchmarks.
> 
> Could you show more details (results)?

In terms of existing benchmarks:

* For app_fib benchmark about 1-3% decrease.
* vm_send and vm_send_var benchmark improves 3-5% due to the send optimization.

I'll try to do some more benchmarking tomorrow and report back.  Is there a decent real world benchmark in `benchmarks` I can use?  With the patch set, some microbenchmarks are slower, but some cases I optimized (bmethod/send/symproc/method_missing) are much faster (over 2x).  A real world benchmark would be more useful to determine the actual performance differences.

I do most of my development on OpenBSD, which is a bit suboptimal for benchmarking small differences in performance in my experience (possibly due to the additional randomization).

If someone could run yjit-bench on the pull request branch (in interpreter mode), that would be very helpful.

> Do you have an analysis which line(s) makes slower?

Unfortunately, I don't.  My guess would be it is due to the additional branches in `CALLER_SETUP_ARG` and checking for `calling->heap_argv`.

> I don't think this feature should be rejected. It is cool to support this feature (long splat can be accepted by rest argument).
> However, personally speaking I feel the proposed patch (https://github.com/ruby/ruby/pull/7522) is too complex for future maintenance comparing with the benefits from the patch.

Agreed.  I wish the patch could be made simpler, but I think most of the complexity of the patch is necessary if we want to fix the bug.

> Now I have no time to review the patch closely and I couldn't confirm this patch has such issue.
> So I agree to merge it (and rewrite them if they can be more improved) because it is well tested.

OK.  Before it is merged, the yjit team needs to make the necessary changes to yjit to support it. Alternatively, they could temporarily disable parts of yjit this breaks, but from talking to @alanwu, that could result in temporarily disabling a lot of yjit.  I think it would be preferable to fix yjit before this is merged.

> I think it is better to have benchmark measurements on some benchmarks, though.

I added some benchmarks related to the optimizations I added, and basic results of those benchmarks in the in related commits.  As mentioned above, I'll try to do additional benchmarking and report back.

> General comments:
> * Some code are duplicated so maybe they can be more shorter.

OK.  I will review and see if I can eliminate the redundant code.

> * (optimization) It is not sure how the optimization target cases are there (optimizations for the minor cases can introduce issues such as i-cache miss, difficulty on future maintenance and so on).

The bmethod/send/symproc/method_missing optimizations are all very large in certain cases and should definitely be included.

The cfunc optimizations are limited to specific cases (`*args` or `*args, **kw` with empty `kw`) and not as large even in those cases (10-15% for `*args`, 35-40% for `*args, **kw` with empty `kw`).  I'm guessing they are still a net performance improvement, though.

----------------------------------------
Bug #4040: SystemStackError with Hash[*a] for Large _a_
https://bugs.ruby-lang.org/issues/4040#change-102795

* Author: runpaint (Run Paint Run Run)
* Status: Open
* Priority: Normal
* Assignee: ko1 (Koichi Sasada)
* ruby -v: ruby 1.9.3dev (2010-11-09 trunk 29737) [x86_64-linux] 
* Backport: 2.2: UNKNOWN, 2.3: UNKNOWN, 2.4: UNKNOWN
----------------------------------------
=begin
 I've been hesitating over whether to file a ticket about this, so please feel free to close if I've made the wrong choice.
 
 I often use Hash[*array.flatten] in IRB to convert arrays of arrays into hashes. Today I noticed that if the array is big enough, this would raise a SystemStackError. Puzzled, I looked deeper. I assumed I was hitting the maximum number of arguments a method's argc can hold, but realised that the minimum size of the array needed to trigger this exception differed depending on whether I used IRB or not. So, presumably this is indeed exhausting the stack...
 
 In IRB, the following is the minimal reproduction of this problem:
 
   Hash[*130648.times.map{ 1 }]; true
 
 I haven't looked for the minimum value needed with `ruby -e`, but the following reproduces:
 
   ruby -e 'Hash[*1380888.times.map{ 1 }]'
 
 I suppose this isn't technically a bug, but maybe it offers another argument for either #666 or an extension of #3131.
=end




-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- [e-mail]
 To unsubscribe send an email to [e-mail]
 ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/