Skip to content

Conversation

@Link-the-elf
Copy link
Contributor

@Link-the-elf Link-the-elf commented Jul 22, 2023

Before submitting the PR, please make sure you do the following

Sorry for my English. Let's see how long this code is executed and what byte-code it will output:

console.time()

const arr = [1, 2]

function loop() {
  for (let i = 0; i <= 1000000000; i++) {
    const [one, two] = arr
  }
}

loop()

console.timeEnd()

Time - 1.585s
V8 byte code for loop function:

LdaZero
Star0
LdaSmi.ExtraWide [1000000000]
TestLessThanOrEqual r0, [0]
JumpIfFalse [165] (0x3491efd187ae @ 176)
LdaImmutableCurrentContextSlot [2]
ThrowReferenceErrorIfHole [0]
Star3
GetIterator r3, [1], [3]
Star5
GetNamedProperty r5, [1], [5]
Star4
LdaFalse
Star6
Mov , r9
Ldar r6
JumpIfToBooleanTrue [33] (0x3491efd18742 @ 68)
LdaTrue
Star6
CallProperty0 r4, r5, [11]
Star10
JumpIfJSReceiver [7] (0x3491efd18731 @ 51)
CallRuntime [ThrowIteratorResultNotAnObject], r10-r10
GetNamedProperty r10, [2], [9]
JumpIfToBooleanTrue [13] (0x3491efd18742 @ 68)
GetNamedProperty r10, [3], [7]
Star10
LdaFalse
Star6
Ldar r10
Jump [3] (0x3491efd18743 @ 69)
LdaUndefined
Star1
Ldar r6
JumpIfToBooleanTrue [33] (0x3491efd18767 @ 105)
LdaTrue
Star6
CallProperty0 r4, r5, [13]
Star10
JumpIfJSReceiver [7] (0x3491efd18756 @ 88)
CallRuntime [ThrowIteratorResultNotAnObject], r10-r10
GetNamedProperty r10, [2], [9]
JumpIfToBooleanTrue [13] (0x3491efd18767 @ 105)
GetNamedProperty r10, [3], [7]
Star10
LdaFalse
Star6
Ldar r10
Jump [3] (0x3491efd18768 @ 106)
LdaUndefined
Star2
LdaSmi [-1]
Star8
Star7
Jump [5] (0x3491efd18772 @ 116)
Star8
LdaZero
Star7
LdaTheHole
SetPendingMessage
Star9
Ldar r6
JumpIfToBooleanTrue [35] (0x3491efd1879a @ 156)
Mov , r11
GetNamedProperty r5, [4], [15]
JumpIfUndefinedOrNull [26] (0x3491efd1879a @ 156)
Star12
CallProperty0 r12, r5, [17]
JumpIfJSReceiver [19] (0x3491efd1879a @ 156)
Star13
CallRuntime [ThrowIteratorResultNotAnObject], r13-r13
Jump [11] (0x3491efd1879a @ 156)
Star11
LdaZero
TestReferenceEqual r7
JumpIfTrue [5] (0x3491efd1879a @ 156)
Ldar r11
ReThrow
Ldar r9
SetPendingMessage
LdaZero
TestReferenceEqual r7
JumpIfFalse [5] (0x3491efd187a5 @ 167)
Ldar r8
ReThrow
Ldar r0
Inc [19]
Star0
JumpLoop [170], [0], [20] (0x3491efd18700 @ 2)
LdaUndefined
Return

Just an awful lot, isn't it? How could we optimize these instructions for V8? Everything is pretty simple. Many people forget that an array is the most common object, and destructuring can be done using { } and not [ ].

By doing this, we will significantly simplify life in V8, since we will tell him exactly from which indices to get values.

Now let's take a look at the optimized version of working on indexes:

console.time()

const arr = [1, 2]

function loop() {
  for (let i = 0; i <= 1000000000; i++) {
    const {0: one, 1: two} = arr
  }
}

loop()

console.timeEnd()

Time - 480.689ms - Code is ~4 times faster
V8 byte code for loop function:

LdaZero
Star0
LdaSmi.ExtraWide [1000000000]
TestLessThanOrEqual r0, [0]
JumpIfFalse [29] (0x70000186d6 @ 40)
LdaImmutableCurrentContextSlot [2]
ThrowReferenceErrorIfHole [0]
Star3
LdaZero
Star4
GetKeyedProperty r3, [1]
Star1
LdaSmi [1]
Star4
GetKeyedProperty r3, [3]
Star2
Ldar r0
Inc [5]
Star0
JumpLoop [34], [0], [6] (0x70000186b0 @ 2)
LdaUndefined
Return

I think it is obvious that the code above for V8 is much easier to process and optimize.

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

@changeset-bot
Copy link

changeset-bot bot commented Jul 22, 2023

⚠️ No Changeset found

Latest commit: 71418f6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Link-the-elf
Copy link
Contributor Author

@hackape Is there anything else required of me?

@hackape
Copy link
Contributor

hackape commented Jul 24, 2023

@Link-the-elf Sorry for causing confusion. I’m just a random contributor like you. I don’t have merge permission. But your PR definitely teaches me sth, I like it!

@benmccann benmccann added the perf label Jul 26, 2023
@benmccann benmccann changed the title feat: Optimazing flip function perf: optimize flip function Jul 26, 2023
@benmccann
Copy link
Member

I'm not sure how I feel about this one. It seems less readable and is in a location where it will be executed only a single time having negligible perf impact unlike in the benchmark in the PR description where it was ran 1B times, so I'm not sure the trade-off is worth it here. I think if we were going to do it we should also audit the codebase and do it everywhere or places with high impact instead of just this one location as there's probably other places it could apply

@dummdidumm
Copy link
Member

Largely agree with Ben here, this is only called one single time on setup.

@benmccann
Copy link
Member

I'm going to go ahead and close this then

@benmccann benmccann closed this Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants