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

Benchmark caveat #6903

Closed
6 tasks done
yyx990803 opened this issue Nov 14, 2024 · 2 comments
Closed
6 tasks done

Benchmark caveat #6903

yyx990803 opened this issue Nov 14, 2024 · 2 comments
Labels
duplicate This issue or pull request already exists

Comments

@yyx990803
Copy link

Describe the bug

When benchmarking vuejs/core#12349, we discovered that due to the way Vitest evaluates modules, there is a slight overhead on every access of import bindings, probably due to the fact that ssrTransform transforms exports into getters:

// a.js
export let a = 1

// ---
// b.js
import { a } from './a'

// every access of `a` here has an getter overhead
console.log(a)
console.log(a)
console.log(a)

export let a = 1 is transformed to

let a = 1
Object.defineProperty(__vite_ssr_exports__, 'a', {
  // ...
  get() { return a }
})

However, for a library like Vue, the actual distributed file is bundled, so a will be scope-hoisted as a local variable in the same scope with the code in b.js, and has no getter overhead.

In hotpaths where a is accessed many times, this overhead can lead to misleading benchmark results. For example, it was confusing at first why instead of exporting a directly, nesting it under an object / namespace actually made it faster.

I'm not sure if there is an actual fix to this in Vitest - but at the very least we should document this caveat in the benchmark docs.

Reproduction

vuejs/core#12349 (comment)

System Info

System:
    OS: macOS 15.0.1
    CPU: (12) arm64 Apple M2 Pro
    Memory: 473.03 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.17.0 - ~/Library/Caches/fnm_multishells/15964_1730078415592/bin/node
    npm: 10.8.2 - ~/Library/Caches/fnm_multishells/15964_1730078415592/bin/npm
    pnpm: 9.12.3 - ~/Library/Caches/fnm_multishells/15964_1730078415592/bin/pnpm
    bun: 1.1.34 - ~/.bun/bin/bun
  Browsers:
    Chrome: 130.0.6723.117
    Chrome Canary: 133.0.6835.0
    Safari: 18.0.1
    Safari Technology Preview: 18.0
  npmPackages:
    @vitest/coverage-v8: ^2.1.1 => 2.1.1
    @vitest/eslint-plugin: ^1.0.1 => 1.1.6
    vite: catalog: => 5.4.0
    vitest: ^2.1.1 => 2.1.1

Used Package Manager

pnpm

Validations

@sheremet-va
Copy link
Member

sheremet-va commented Nov 14, 2024

This is a duplicate of #6543. The team agrees that it should be documented.

I do wonder if instead of having a getter we can just reassign the value on the __vite_ssr_exports__ when it is changed by the user.

@sheremet-va sheremet-va added duplicate This issue or pull request already exists and removed pending triage labels Nov 14, 2024
@yyx990803
Copy link
Author

Oh ok, closing as duplicate then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants