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.

Sign up for GitHub

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

[perf] Improve SchedulerMinHeap implementation #21147

Merged
merged 2 commits into from
Apr 6, 2021
Merged

[perf] Improve SchedulerMinHeap implementation #21147

merged 2 commits into from
Apr 6, 2021

Conversation

Tom910
Copy link
Contributor

@Tom910 Tom910 commented Mar 30, 2021 •
edited
Loading

Migrated to a new algorithm that does not go beyond the array, thus de-optimization of this piece of code is not happening

Summary

I investigated the package scheduler and observed that the current implementation MinHeap reading beyond the length of the array. MinHeap is hot type code and I had decided to improve the current implementation

Result

❯ node -v
v14.15.5

❯ node bench.js
master branche - small number of elements x 17,740 ops/sec ±1.50% (85 runs sampled)
master branche - large number of elements x 1,430 ops/sec ±1.10% (71 runs sampled)

current branche - small number of elements x 441,482 ops/sec ±0.60% (95 runs sampled)
current branche - large number of elements x 14,038 ops/sec ±1.17% (91 runs sampled)

Proofs why it's work that

reading array[42] when array.length === 5. In this case, the array index 42 is out of bounds, the property is not present on the array itself, and so the JavaScript engine has to perform expensive prototype chain lookups. Once a load has run into this situation, V8 remembers that “this load needs to deal with special cases”, and it will never be as fast again as it was before reading out-of-bounds.

url - https://v8.dev/blog/elements-kinds#avoid-reading-beyond-the-length-of-the-array

Test Plan

@sizebot
Copy link

sizebot commented Mar 30, 2021 •
edited
Loading

Comparing: 64983aa...49d3249

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 122.30 kB 122.30 kB = 39.25 kB 39.25 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 128.81 kB 128.81 kB = 41.33 kB 41.33 kB
facebook-www/ReactDOM-prod.classic.js = 404.53 kB 404.53 kB = 75.21 kB 75.21 kB
facebook-www/ReactDOM-prod.modern.js = 392.79 kB 392.79 kB = 73.33 kB 73.33 kB
facebook-www/ReactDOMForked-prod.classic.js = 404.53 kB 404.53 kB = 75.21 kB 75.21 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/Scheduler-dev.classic.js = 23.69 kB 23.63 kB +0.02% 6.33 kB 6.34 kB
facebook-www/Scheduler-dev.modern.js = 23.69 kB 23.63 kB +0.02% 6.33 kB 6.34 kB
oss-experimental/react/umd/react.profiling.min.js = 14.29 kB 14.26 kB = 5.28 kB 5.28 kB
facebook-www/SchedulerPostTaskOnly-dev.classic.js = 22.43 kB 22.38 kB +0.07% 5.95 kB 5.95 kB
facebook-www/SchedulerPostTaskOnly-dev.modern.js = 22.43 kB 22.38 kB +0.07% 5.95 kB 5.95 kB
facebook-www/SchedulerMock-dev.classic.js = 22.20 kB 22.15 kB = 5.25 kB 5.25 kB
facebook-www/SchedulerMock-dev.modern.js = 22.20 kB 22.15 kB = 5.25 kB 5.25 kB
oss-stable/react/umd/react.profiling.min.js = 13.24 kB 13.21 kB = 4.98 kB 4.98 kB
oss-experimental/react/umd/react.production.min.js = 12.07 kB 12.04 kB = 4.72 kB 4.72 kB
oss-experimental/scheduler/umd/scheduler-unstable_mock.development.js = 18.62 kB 18.56 kB = 4.28 kB 4.27 kB
oss-stable/scheduler/umd/scheduler-unstable_mock.development.js = 18.62 kB 18.56 kB = 4.28 kB 4.27 kB
oss-stable/react/umd/react.production.min.js = 11.02 kB 10.99 kB +0.11% 4.43 kB 4.43 kB
facebook-www/SchedulerNoDOM-dev.classic.js = 17.89 kB 17.83 kB = 4.54 kB 4.54 kB
facebook-www/SchedulerNoDOM-dev.modern.js = 17.89 kB 17.83 kB = 4.54 kB 4.54 kB
facebook-react-native/scheduler/cjs/SchedulerMock-dev.js = 17.35 kB 17.30 kB = 4.16 kB 4.15 kB
oss-experimental/scheduler/cjs/scheduler-unstable_mock.development.js = 17.31 kB 17.25 kB = 4.17 kB 4.16 kB
oss-stable/scheduler/cjs/scheduler-unstable_mock.development.js = 17.31 kB 17.25 kB = 4.17 kB 4.16 kB
facebook-react-native/scheduler/cjs/Scheduler-dev.js = 17.20 kB 17.15 kB = 4.88 kB 4.88 kB
oss-experimental/scheduler/cjs/scheduler.development.js = 17.16 kB 17.10 kB = 4.89 kB 4.88 kB
oss-stable/scheduler/cjs/scheduler.development.js = 17.16 kB 17.10 kB = 4.89 kB 4.88 kB
oss-experimental/scheduler/cjs/scheduler-unstable_post_task_only.development.js = 15.94 kB 15.89 kB = 4.51 kB 4.51 kB
oss-stable/scheduler/cjs/scheduler-unstable_post_task_only.development.js = 15.94 kB 15.89 kB = 4.51 kB 4.51 kB
oss-experimental/scheduler/cjs/scheduler-unstable_no_dom.development.js = 13.07 kB 13.02 kB = 3.46 kB 3.46 kB
oss-stable/scheduler/cjs/scheduler-unstable_no_dom.development.js = 13.07 kB 13.02 kB = 3.46 kB 3.46 kB
facebook-react-native/scheduler/cjs/SchedulerNoDOM-dev.js = 13.03 kB 12.97 kB = 3.44 kB 3.43 kB
facebook-www/Scheduler-profiling.classic.js = 15.04 kB 14.97 kB +0.17% 3.55 kB 3.56 kB
facebook-www/Scheduler-profiling.modern.js = 15.04 kB 14.97 kB +0.17% 3.55 kB 3.56 kB
facebook-www/SchedulerPostTaskOnly-profiling.classic.js = 14.35 kB 14.29 kB +0.15% 3.42 kB 3.42 kB
facebook-www/SchedulerPostTaskOnly-profiling.modern.js = 14.35 kB 14.29 kB +0.15% 3.42 kB 3.42 kB
facebook-www/SchedulerNoDOM-profiling.classic.js = 12.54 kB 12.48 kB +0.07% 2.95 kB 2.95 kB
facebook-www/SchedulerNoDOM-profiling.modern.js = 12.54 kB 12.48 kB +0.07% 2.95 kB 2.95 kB
facebook-www/SchedulerMock-prod.classic.js = 12.03 kB 11.96 kB +0.04% 2.85 kB 2.85 kB
facebook-www/SchedulerMock-prod.modern.js = 12.03 kB 11.96 kB +0.04% 2.85 kB 2.85 kB
facebook-react-native/scheduler/cjs/SchedulerMock-prod.js = 11.76 kB 11.70 kB +0.04% 2.77 kB 2.77 kB
facebook-www/Scheduler-prod.classic.js = 11.33 kB 11.27 kB +0.04% 2.86 kB 2.86 kB
facebook-www/Scheduler-prod.modern.js = 11.33 kB 11.27 kB +0.04% 2.86 kB 2.86 kB
facebook-www/SchedulerPostTaskOnly-prod.classic.js = 10.66 kB 10.60 kB = 2.73 kB 2.73 kB
facebook-www/SchedulerPostTaskOnly-prod.modern.js = 10.66 kB 10.60 kB = 2.73 kB 2.73 kB
facebook-react-native/scheduler/cjs/Scheduler-prod.js = 10.39 kB 10.33 kB = 2.65 kB 2.65 kB
facebook-react-native/scheduler/cjs/Scheduler-profiling.js = 10.39 kB 10.33 kB = 2.65 kB 2.65 kB
oss-experimental/scheduler/umd/scheduler-unstable_mock.production.min.js = 4.81 kB 4.77 kB +0.30% 2.02 kB 2.02 kB
oss-stable/scheduler/umd/scheduler-unstable_mock.production.min.js = 4.81 kB 4.77 kB +0.30% 2.02 kB 2.02 kB
oss-experimental/scheduler/cjs/scheduler-unstable_mock.production.min.js = 4.76 kB 4.73 kB = 1.92 kB 1.92 kB
oss-stable/scheduler/cjs/scheduler-unstable_mock.production.min.js = 4.76 kB 4.73 kB = 1.92 kB 1.92 kB
facebook-www/SchedulerNoDOM-prod.classic.js = 8.83 kB 8.77 kB = 2.24 kB 2.24 kB
facebook-www/SchedulerNoDOM-prod.modern.js = 8.83 kB 8.77 kB = 2.24 kB 2.24 kB
oss-experimental/scheduler/cjs/scheduler.production.min.js = 4.41 kB 4.37 kB = 1.84 kB 1.84 kB
oss-stable/scheduler/cjs/scheduler.production.min.js = 4.41 kB 4.37 kB = 1.84 kB 1.84 kB
facebook-react-native/scheduler/cjs/SchedulerNoDOM-prod.js = 8.58 kB 8.52 kB = 2.16 kB 2.16 kB
facebook-react-native/scheduler/cjs/SchedulerNoDOM-profiling.js = 8.58 kB 8.52 kB = 2.16 kB 2.16 kB
oss-experimental/scheduler/cjs/scheduler-unstable_post_task_only.production.min.js = 4.08 kB 4.05 kB = 1.77 kB 1.76 kB
oss-stable/scheduler/cjs/scheduler-unstable_post_task_only.production.min.js = 4.08 kB 4.05 kB = 1.77 kB 1.76 kB
oss-experimental/scheduler/cjs/scheduler-unstable_no_dom.production.min.js = 3.54 kB 3.51 kB = 1.48 kB 1.48 kB
oss-stable/scheduler/cjs/scheduler-unstable_no_dom.production.min.js = 3.54 kB 3.51 kB = 1.48 kB 1.48 kB

Generated by 🚫 dangerJS against 49d3249

yisar
yisar approved these changes Apr 1, 2021
View reviewed changes
packages/scheduler/src/SchedulerMinHeap.js Outdated Show resolved Hide resolved
@Tom910
Improve MinHeap implementation
5fb47c2 
Migrated to a new algorithm that does not go beyond the array, thus de-optimization of this piece of code is not happening
@sebmarkbage sebmarkbage requested a review from acdlite April 1, 2021 14:30
@acdlite
Copy link
Collaborator

acdlite commented Apr 6, 2021

Thanks for the PR! And for measuring the changes.

I think the original implementation used type checks to satisfy Flow (even though I forgot Flow intentionally pretends accessing an array of type Array<T> results in a T, since otherwise you have to add a bunch of unnecessary checks when doing loops), and then since it already checked the type, it didn't bother to also check the index. So, good catch!

I reverted the changes that weren't related to preventing out-of-bounds access, since they didn't change the algorithm. I don't really object to any of those changes, just in general I prefer not to merge stylistic changes since it introduces additional extra risk. Especially this code which is in an extremely hot path.

The swap function arguably aids readability but I prefer to keep that inlined so that we don't have to rely on Closure to inline it. (Even though Closure is gonna inline it anyway.)

@acdlite acdlite merged commit 316aa36 into facebook:master Apr 6, 2021
@Tom910 Tom910 deleted the improve-heap-algorithm branch April 6, 2021 13:20
acdlite added a commit to acdlite/react that referenced this pull request Apr 11, 2021
@Tom910 @acdlite
[Scheduler] Fix de-opt caused by out-of-bounds access (facebook#21147)
bdf105b 
Scheduler's heap implementation sometimes accesses indices that are out
of bounds (larger than the size of the array). This causes a VM de-opt.

This change fixes the de-opt by always checking the index before
accessing the array. In exchange, we can remove the typecheck on the
returned element.

Background: https://v8.dev/blog/elements-kinds#avoid-reading-beyond-the-length-of-the-array

Co-authored-by: Andrew Clark <git@andrewclark.io>
acdlite added a commit to acdlite/react that referenced this pull request Apr 13, 2021
@Tom910 @acdlite
[Scheduler] Fix de-opt caused by out-of-bounds access (facebook#21147)
0a6c9a3 
Scheduler's heap implementation sometimes accesses indices that are out
of bounds (larger than the size of the array). This causes a VM de-opt.

This change fixes the de-opt by always checking the index before
accessing the array. In exchange, we can remove the typecheck on the
returned element.

Background: https://v8.dev/blog/elements-kinds#avoid-reading-beyond-the-length-of-the-array

Co-authored-by: Andrew Clark <git@andrewclark.io>
acdlite added a commit to acdlite/react that referenced this pull request Apr 16, 2021
@Tom910 @acdlite
[Scheduler] Fix de-opt caused by out-of-bounds access (facebook#21147)
3ee4d64 
Scheduler's heap implementation sometimes accesses indices that are out
of bounds (larger than the size of the array). This causes a VM de-opt.

This change fixes the de-opt by always checking the index before
accessing the array. In exchange, we can remove the typecheck on the
returned element.

Background: https://v8.dev/blog/elements-kinds#avoid-reading-beyond-the-length-of-the-array

Co-authored-by: Andrew Clark <git@andrewclark.io>
acdlite added a commit to acdlite/react that referenced this pull request Apr 16, 2021
@Tom910 @acdlite
[Scheduler] Fix de-opt caused by out-of-bounds access (facebook#21147)
47a4ee7 
Scheduler's heap implementation sometimes accesses indices that are out
of bounds (larger than the size of the array). This causes a VM de-opt.

This change fixes the de-opt by always checking the index before
accessing the array. In exchange, we can remove the typecheck on the
returned element.

Background: https://v8.dev/blog/elements-kinds#avoid-reading-beyond-the-length-of-the-array

Co-authored-by: Andrew Clark <git@andrewclark.io>
acdlite added a commit to acdlite/react that referenced this pull request Apr 19, 2021
@Tom910 @acdlite
[Scheduler] Fix de-opt caused by out-of-bounds access (facebook#21147)
790147d 
Scheduler's heap implementation sometimes accesses indices that are out
of bounds (larger than the size of the array). This causes a VM de-opt.

This change fixes the de-opt by always checking the index before
accessing the array. In exchange, we can remove the typecheck on the
returned element.

Background: https://v8.dev/blog/elements-kinds#avoid-reading-beyond-the-length-of-the-array

Co-authored-by: Andrew Clark <git@andrewclark.io>
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Apr 20, 2021
@facebook-github-bot
React Native sync for revisions c9aab1c...f7cdc89
de318cd 
Summary:
This sync includes the following changes:
- **[f7cdc8936](facebook/react@f7cdc8936 )**: Also turn off enableSyncDefaultUpdates in RN test renderer ([#21293](facebook/react#21293)) //<Ricky>//
- **[4c9eb2af1](facebook/react@4c9eb2af1 )**: Add dynamic flags to React Native ([#21291](facebook/react#21291)) //<Ricky>//
- **[9eddfbf5a](facebook/react@9eddfbf5a )**: [Fizz] Two More Fixes ([#21288](facebook/react#21288)) //<Sebastian Markbåge>//
- **[11b07597e](facebook/react@11b07597e )**: Fix classes ([#21283](facebook/react#21283)) //<Sebastian Markbåge>//
- **[96d00b9bb](facebook/react@96d00b9bb )**: [Fizz] Random Fixes ([#21277](facebook/react#21277)) //<Sebastian Markbåge>//
- **[81ef53953](facebook/react@81ef53953 )**: Always insert a dummy node with an ID into fallbacks ([#21272](facebook/react#21272)) //<Sebastian Markbåge>//
- **[a4a940d7a](facebook/react@a4a940d7a )**: [Fizz] Add unsupported Portal/Scope components ([#21261](facebook/react#21261)) //<Sebastian Markbåge>//
- **[f4d7a0f1e](facebook/react@f4d7a0f1e )**: Implement useOpaqueIdentifier ([#21260](facebook/react#21260)) //<Sebastian Markbåge>//
- **[dde875dfb](facebook/react@dde875dfb )**: [Fizz] Implement Hooks ([#21257](facebook/react#21257)) //<Sebastian Markbåge>//
- **[a597c2f5d](facebook/react@a597c2f5d )**: [Fizz] Fix reentrancy bug ([#21270](facebook/react#21270)) //<Sebastian Markbåge>//
- **[15e779d92](facebook/react@15e779d92 )**: Reconciler should inject its own version into DevTools hook ([#21268](facebook/react#21268)) //<Brian Vaughn>//
- **[4f76a28c9](facebook/react@4f76a28c9 )**: [Fizz] Implement New Context ([#21255](facebook/react#21255)) //<Sebastian Markbåge>//
- **[82ef450e0](facebook/react@82ef450e0 )**: remove obsolete SharedArrayBuffer ESLint config ([#21259](facebook/react#21259)) //<Henry Q. Dineen>//
- **[dbadfa2c3](facebook/react@dbadfa2c3 )**: [Fizz] Classes Follow Up ([#21253](facebook/react#21253)) //<Sebastian Markbåge>//
- **[686b635b7](facebook/react@686b635b7 )**: Prevent reading canonical property of null ([#21242](facebook/react#21242)) //<Joshua Gross>//
- **[bb88ce95a](facebook/react@bb88ce95a )**: Bugfix: Don't rely on `finishedLanes` for passive effects ([#21233](facebook/react#21233)) //<Andrew Clark>//
- **[343710c92](facebook/react@343710c92 )**: [Fizz] Fragments and Iterable support ([#21228](facebook/react#21228)) //<Sebastian Markbåge>//
- **[933880b45](facebook/react@933880b45 )**: Make time-slicing opt-in ([#21072](facebook/react#21072)) //<Ricky>//
- **[b0407b55f](facebook/react@b0407b55f )**: Support more empty types ([#21225](facebook/react#21225)) //<Sebastian Markbåge>//
- **[39713716a](facebook/react@39713716a )**: Merge isObject branches ([#21226](facebook/react#21226)) //<Sebastian Markbåge>//
- **[8a4a59c72](facebook/react@8a4a59c72 )**: Remove textarea special case from child fiber ([#21222](facebook/react#21222)) //<Sebastian Markbåge>//
- **[dc108b0f5](facebook/react@dc108b0f5 )**: Track which fibers scheduled the current render work ([#15658](facebook/react#15658)) //<Brian Vaughn>//
- **[6ea749170](facebook/react@6ea749170 )**: Fix typo in comment ([#21214](facebook/react#21214)) //<inokawa>//
- **[b38ac13f9](facebook/react@b38ac13f9 )**: DevTools: Add post-commit hook ([#21183](facebook/react#21183)) //<Brian Vaughn>//
- **[b943aeba8](facebook/react@b943aeba8 )**: Fix: Passive effect updates are never sync ([#21215](facebook/react#21215)) //<Andrew Clark>//
- **[d389c54d1](facebook/react@d389c54d1 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21211](facebook/react#21211)) //<Brian Vaughn>//
- **[c486dc1a4](facebook/react@c486dc1a4 )**: Remove unnecessary processUpdateQueue ([#21199](facebook/react#21199)) //<Sebastian Markbåge>//
- **[cf45a623a](facebook/react@cf45a623a )**: [Fizz] Implement Classes ([#21200](facebook/react#21200)) //<Sebastian Markbåge>//
- **[75c616554](facebook/react@75c616554 )**: Include actual type of `Profiler#id` on type mismatch ([#20306](facebook/react#20306)) //<Sebastian Silbermann>//
- **[1214b302e](facebook/react@1214b302e )**: test: Fix "couldn't locate all inline snapshots" ([#21205](facebook/react#21205)) //<Sebastian Silbermann>//
- **[1a02d2792](facebook/react@1a02d2792 )**: style: delete unused isHost check ([#21203](facebook/react#21203)) //<wangao>//
- **[782f689ca](facebook/react@782f689ca )**: Don't double invoke getDerivedStateFromProps for module pattern ([#21193](facebook/react#21193)) //<Sebastian Markbåge>//
- **[e90c76a65](facebook/react@e90c76a65 )**: Revert "Offscreen: Use JS stack to track hidden/unhidden subtree state" ([#21194](facebook/react#21194)) //<Brian Vaughn>//
- **[1f8583de8](facebook/react@1f8583de8 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21192](facebook/react#21192)) //<Brian Vaughn>//
- **[ad6e6ec7b](facebook/react@ad6e6ec7b )**: [Fizz] Prepare Recursive Loop for More Types ([#21186](facebook/react#21186)) //<Sebastian Markbåge>//
- **[172e89b4b](facebook/react@172e89b4b )**: Reland Remove redundant initial of isArray ([#21188](facebook/react#21188)) //<Sebastian Markbåge>//
- **[7c1ba2b57](facebook/react@7c1ba2b57 )**: Proposed new Suspense layout effect semantics ([#21079](facebook/react#21079)) //<Brian Vaughn>//
- **[316aa3686](facebook/react@316aa3686 )**: [Scheduler] Fix de-opt caused by out-of-bounds access ([#21147](facebook/react#21147)) //<Andrey Marchenko>//
- **[b4f119cdf](facebook/react@b4f119cdf )**: Revert "Remove redundant initial of isArray ([#21163](facebook/react#21163))" //<Sebastian Markbage>//
- **[c03197063](facebook/react@c03197063 )**: Revert "apply prettier ([#21165](facebook/react#21165))" //<Sebastian Markbage>//
- **[94fd1214d](facebook/react@94fd1214d )**: apply prettier ([#21165](facebook/react#21165)) //<Behnam Mohammadi>//
- **[b130a0f5c](facebook/react@b130a0f5c )**: Remove redundant initial of isArray ([#21163](facebook/react#21163)) //<Behnam Mohammadi>//
- **[2c9fef32d](facebook/react@2c9fef32d )**: Remove redundant initial of hasOwnProperty ([#21134](facebook/react#21134)) //<Behnam Mohammadi>//
- **[1cf9978d8](facebook/react@1cf9978d8 )**: Don't pass internals to callbacks ([#21161](facebook/react#21161)) //<Sebastian Markbåge>//
- **[b9e4c10e9](facebook/react@b9e4c10e9 )**: [Fizz] Implement all the DOM attributes and special cases ([#21153](facebook/react#21153)) //<Sebastian Markbåge>//
- **[f8ef4ff57](facebook/react@f8ef4ff57 )**: Flush discrete passive effects before paint ([#21150](facebook/react#21150)) //<Andrew Clark>//
- **[b48b38af6](facebook/react@b48b38af6 )**: Support nesting of startTransition and flushSync (alt) ([#21149](facebook/react#21149)) //<Sebastian Markbåge>//

Changelog:
[General][Changed] - React Native sync for revisions c9aab1c...f7cdc89

jest_e2e[run_all_tests]

Reviewed By: rickhanlonii

Differential Revision: D27740113

fbshipit-source-id: 6e27204d78e3e16ed205170006cb97c0d6bfa957
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
@Tom910 @acdlite @koto
[Scheduler] Fix de-opt caused by out-of-bounds access (facebook#21147)
b63966d 
Scheduler's heap implementation sometimes accesses indices that are out
of bounds (larger than the size of the array). This causes a VM de-opt.

This change fixes the de-opt by always checking the index before
accessing the array. In exchange, we can remove the typecheck on the
returned element.

Background: https://v8.dev/blog/elements-kinds#avoid-reading-beyond-the-length-of-the-array

Co-authored-by: Andrew Clark <git@andrewclark.io>
eps1lon
eps1lon reviewed Mar 19, 2022
View reviewed changes
packages/scheduler/src/SchedulerMinHeap.js
@@ -58,15 +56,16 @@ function siftUp(heap, node, i) {
function siftDown(heap, node, i) {
let index = i;
const length = heap.length;
while (index < length) {
const halfLength = length >>> 1;
while (index < halfLength) {
const leftIndex = (index + 1) * 2 - 1;
const left = heap[leftIndex];
const rightIndex = leftIndex + 1;
const right = heap[rightIndex];
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Tom910 Isn't the algorithm going beyond the array length here potentially? Otherwise we wouldn't need to check if rightIndex < length later, right?

Or does v8 only actually go through the prototype chain once we actually use the value e.g. when doing right !== undefined?

Or is de-opting here fine? You mentioned benchmarks in your PR description but I couldn't find where they are located. Could you share them for (future) testing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eps1lon eps1lon eps1lon left review comments

@yisar yisar yisar approved these changes

@acdlite acdlite Awaiting requested review from acdlite

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Tom910 @sizebot @acdlite @eps1lon @yisar @facebook-github-bot