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

Improve Array#shift performance on v8 > 7.1 (Node 12+) #2115

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

ggrossetie
Copy link
Member

Command

$ bundle exec rake bench:ips FILE=benchmark-ips/bm_array_shift.rb

Before

Node 10

=== Files: benchmark-ips/bm_array_shift.rb
bundle exec opal --dynamic-require ignore --missing-require ignore -rnodejs -ropal/platform -gbenchmark-ips -rbenchmark/ips -A benchmark-ips/bm_array_shift.rb
""
Warming up --------------------------------------
        shift no arg    39.327k i/100ms
Calculating -------------------------------------
        shift no arg    979.605k (± 1.6%) i/s -      4.916M in   5.019468s

Node 12


=== Files: benchmark-ips/bm_array_shift.rb
bundle exec opal --dynamic-require ignore --missing-require ignore -rnodejs -ropal/platform -gbenchmark-ips -rbenchmark/ips -A benchmark-ips/bm_array_shift.rb
""
Warming up --------------------------------------
        shift no arg     2.364k i/100ms
Calculating -------------------------------------
        shift no arg     24.879k (± 0.5%) i/s -    125.292k in   5.036277s

After

Node 10

=== Files: benchmark-ips/bm_array_shift.rb
bundle exec opal --dynamic-require ignore --missing-require ignore -ropal/platform -gbenchmark-ips -rbenchmark/ips -A benchmark-ips/bm_array_shift.rb
Warming up --------------------------------------
        shift no arg    26.398k i/100ms
Calculating -------------------------------------
        shift no arg    449.226k (± 4.4%) i/s -      2.244M in   5.005441s

Node 12

=== Files: benchmark-ips/bm_array_shift.rb
bundle exec opal --dynamic-require ignore --missing-require ignore -ropal/platform -gbenchmark-ips -rbenchmark/ips -A benchmark-ips/bm_array_shift.rb
Warming up --------------------------------------
        shift no arg    37.305k i/100ms
Calculating -------------------------------------
        shift no arg    611.658k (± 1.0%) i/s -      3.059M in   5.001669s

Summary

With this change, the code is now 2.18x times slower when using Node 10 but 24.58x times faster when using Node 12.

We cannot beat the highly optimized code produced by v8 pre 7.1 (Node 10) but the code is now "only" 1.6x times slower when using a recent version of v8/Node.

Please note that the performance on Node 14 is roughly equivalent to Node 12.

Further improvement

The Array#shift() (JavaScript) function is also used in String#tr and String#tr_s. It might be worth to replace this call by the optimized shiftNoArg function? In this case, where should I declare the shiftNoArg function? in runtime.js?

I also found an optimization for splice when the second argument is 1 (ie. list.splice(position, 1)). I found 8 occurrences in the code where the second argument is explicitly 1. When a variable is used as the second argument it might be worth to check if the value is actually 1 to use the optimized code (I think it's only worth if the array is large enough but that means that we need to add an additional check that will slow down the execution).

opal/corelib/array.rb Show resolved Hide resolved
@ggrossetie
Copy link
Member Author

ggrossetie commented Oct 11, 2020

@elia I found a more generic implementation:

function spliceGen (list, start, deleteCount) {
  if (deleteCount <= 0) {
    return
  }
  const length = list.length
  for (var i = start, k = i + deleteCount; k < length; i += 1, k += 1)
    list[i] = list[k]

  list.length = i
  // or:
  //for (;i < length; i++)
  //  list.pop()
}

Depending on the list length, calling list.pop() can be faster than updating the list length. The goal is to remove "trailing" elements.

Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching and taking care of this!
I left one nit, let me know if you don't have time to update the PR and I'll merge it as it is 👍

opal/corelib/array.rb Outdated Show resolved Hide resolved
@ggrossetie
Copy link
Member Author

@elia I will keep the focus of this pull request to just Array#shift and open a new pull request to generalize the usage of spliceOne / spliceGen.

@ggrossetie
Copy link
Member Author

@elia I've added a changelog entry and rebased/squashed my work. Let me know if there's anything left to do.

@elia elia merged commit df69b8e into opal:master Oct 30, 2020
@ggrossetie ggrossetie deleted the perf-splice branch October 30, 2020 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants