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

optimization in flatten function #2499

Closed
wants to merge 1 commit into from

Conversation

marijaselakovic
Copy link
Contributor

Similarly as in https://github.com/jashkenas/underscore/pull/2496, I found that flatten function can be optimized by swapping the order of checks in logical expression in line 500 (the second check is false most of the time). I provide the screenshot with the results for unit tests which performance got improved (on V8 4.4 engine). Inputs are the same as in unit tests in the repository and performance is tested with Benchmark.js library.
selection_012

The code for tests can be download from: https://s3.eu-central-1.amazonaws.com/underscorebenchmark/benchmarks2.zip

To run the tests, just call

node run.js

I am maybe wrong, but can isArrayLike(value) check be omitted here?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.863% when pulling f126f19 on marijaselakovic:master into 5500925 on jashkenas:master.

@megawac
Copy link
Collaborator

megawac commented May 2, 2016

Yes, isArrayLike can be removed here

@jridgewell
Copy link
Collaborator

Yes, isArrayLike can be removed here

Not quite. This opt will only benefit isArray since it's faster than isArrayLike + isArray on non-array objects. But this'll hurt the isArguments check, since that uses the slower toString comparison.

Instead, we should guard only the isArguments check:

if (_.isArray(value) || (isArrayLike(value) && _.isArguments(value)) {
  //...
}

@jgonggrijp
Copy link
Collaborator

isArrayLike could be removed, as it is only there to improve performance. The current check is already optimal, however:

  arrays arguments other
current isArrayLike + isArray isArrayLike + isArray + isArguments isArrayLike
marijaselakovic isArray + isArrayLike isArray + isArguments + isArrayLike isArray + isArguments + isArrayLike
megawac isArray isArray + isArguments isArray + isArguments
jridgewell isArray isArray + isArrayLike + `isArguments isArray + isArrayLike

The toString-based checks isArray and isArguments are more expensive than the heuristic isArrayLike. See the comments in the resolved part of the second review in #2840. Avoiding isArrayLike for arrays is only a small optimization while having isArray for other values is a big pessimization.

@jgonggrijp jgonggrijp closed this May 9, 2020
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.

None yet

5 participants