Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

use concat(+join) for += #2256

Closed
mpapis opened this issue Dec 1, 2014 · 7 comments
Closed

use concat(+join) for += #2256

mpapis opened this issue Dec 1, 2014 · 7 comments
Milestone
Invalid or Duplicate

Comments

@mpapis
Copy link
Contributor

mpapis commented Dec 1, 2014

at start I want to accent this is also slow in MRI:

require 'benchmark'

rep = 10_000

Benchmark.bmbm do |x|
  {
    1..25 => [],
    "a".."z" => "",
  }.each do |base, storage|

    base = base.to_a
    basej = base
    class_name = storage.class.to_s

    x.report(class_name+'#concat') do
      a = storage.dup
      basej = base.join if storage == ""
      rep.times { a.concat(basej) }
    end

    x.report(class_name+'#<<') do
      a = storage.dup
      basej = base.join if storage == ""
      rep.times { base.each { |e| a << e } }
    end

    x.report(class_name+'#+=') do
      a = storage.dup
      basej = base.join if storage == ""
      rep.times { a += basej }
    end

  end
end

with result:

                    user     system      total        real
Array#concat    0.010000   0.000000   0.010000 (  0.004000)
Array#<<        0.150000   0.000000   0.150000 (  0.051000)
Array#+=        0.850000   0.010000   0.860000 (  0.696000)
String#concat   0.000000   0.000000   0.000000 (  0.001000)
String#<<       0.070000   0.000000   0.070000 (  0.023000)
String#+=       0.230000   0.000000   0.230000 (  0.181000)

I understand that the []+[] has to create copy of array, but for []+=[] I expect it to behave like concat, not sure if this can or should be optimized.

opening first here, will follow with a ticket for MRI if it can be improved.
This was referenced Dec 1, 2014
Merged
Closed
@mpapis
Copy link
Contributor Author

mpapis commented Dec 1, 2014

after discussing in #rubinius the proper behavior would be dup.concat - which then is slower, but I guess this could be discussed on MRI

@chrisseaton
Copy link
Contributor

Did you benchmark dup.concat as well?

I don't think that a+=b (when arrays) should behave like a.concat(b). When I see a+=b I think a=a+b. I therefore wouldn't expect the original a object to be modified. What you are suggesting adds a new semantic corner-case to Ruby, which is already an incredibly complicated language.

However, I do believe that Python behaves as you're saying you would like Ruby to. I know that because I found that my lists were being mutated when I wasn't expecting them to be!

@mpapis
Copy link
Contributor Author

mpapis commented Dec 1, 2014

as mentioned above I did and it is slower, I think now the simplest move would be to update docs to be more explicit about the differences between += and concat

@headius
Copy link
Member

headius commented Dec 1, 2014

The change you're discussing would be much larger than just modifying Array behavior. Since += expands to a = a + b in the parser/compiler, we'd have to change += to parse as a method call or modify the compilers to emit some special guard logic that can concat when it detects an Array. I don't think either change is likely to be accepted.

The latter change, however, could be done in JRuby or JRuby+Truffle if we added an instruction/node for += that did the guard+optimized logic using JVM capabilities. I'm dubious on the value, since it's generally well-known that += causes more allocation and copying than << or concat.

@mpapis
Copy link
Contributor Author

mpapis commented Dec 1, 2014

I see now it makes not much sense, close if you don't want to work on the optimization

@enebo enebo added this to the Invalid or Duplicate milestone Dec 1, 2014
@enebo
Copy link
Member

enebo commented Dec 1, 2014

I will close this but will also add an optimization thought. If we know origin of 'a' and we know nothing is holding a reference to 'a' we could potentially just change the id on original id and concat! into the original. With that idea out there I think the number of places where these conditions exist might make that effort not worh it...

@enebo enebo closed this as completed Dec 1, 2014
@mpapis
Copy link
Contributor Author

mpapis commented Dec 1, 2014

let's see what MRI does => https://bugs.ruby-lang.org/issues/10560

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Invalid or Duplicate
Development

No branches or pull requests
4 participants
@headius @enebo @mpapis @chrisseaton