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

Math within vm context is extremely slow #49283

Closed
a-parser opened this issue Aug 22, 2023 · 5 comments
Closed

Math within vm context is extremely slow #49283

a-parser opened this issue Aug 22, 2023 · 5 comments
Labels
v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.

Comments

@a-parser
Copy link

Version

14.x,16.x,18.x,20.x

Platform

linux/macos

Subsystem

No response

What steps will reproduce the bug?

Accessing to Math within vm context is about x50-x100 slower vs native execution:

native 0.287
context 18.947
context-builtin-pow 0.292
const vm = require('vm');

function math_benchmark(name, builtinPow = false) {
    const startTime = Date.now();
    for(let i = 1; i <= 100_000_000; i++) {
        let x = builtinPow ? i ** 2 : Math.pow(i, 2);
        if(i % 100_000_000 == 0) {
            x && console.log(name, (Date.now() - startTime) / 1000);
        }
    }
}

math_benchmark('native');

const ctx = {console};
ctx.global = ctx;
vm.createContext(ctx);

vm.runInNewContext(`
    ${math_benchmark.toString()}
    math_benchmark("context")
`, ctx);

vm.runInNewContext(`
    ${math_benchmark.toString()}
    math_benchmark("context-builtin-pow", true)
`, ctx);

How often does it reproduce? Is there a required condition?

always, tested on all node versions from 14 to 20

What is the expected behavior? Why is that the expected behavior?

it's expected to have nearly same performance

What do you see instead?

it's slower x50-x100 times

Additional information

No response

@bnoordhuis
Copy link
Member

Thanks for the report but the slowdown isn't unexpected, it's an inevitable consequence of how the vm module is implemented.

Executive summary: V8 can normally inline and optimize Math method calls but the vm module intercepts all loads and stores from/to the global object, depriving V8 of the possibility of recognizing such built-in methods.

It's why I wrote the vime module years ago, it doesn't suffer from that defect. Closing as a wontfix/cantfix.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Aug 22, 2023
@bnoordhuis bnoordhuis added vm Issues and PRs related to the vm subsystem. v8 engine Issues and PRs related to the V8 dependency. labels Aug 22, 2023
@a-parser
Copy link
Author

@bnoordhuis, Thanks for the explanation, but I'm not agree with "wontfix".

  1. This performance behavior completely undocumented in vm module docs
  2. Math is JavaScript builtin object, seems it not need to be proxied within vm module

@a-parser
Copy link
Author

a-parser commented Aug 22, 2023

there is a workaround by localizing Math

function math_benchmark(name, builtinPow = false) {
    const startTime = Date.now();

    const Math = global.Math;
    for(let i = 1; i <= 100_000_000; i++) {
        let x = builtinPow ? i ** 2 : Math.pow(i, 2);
        if(i % 100_000_000 == 0) {
            x && console.log(name, (Date.now() - startTime) / 1000);
        }
    }
}

but honestly it's looks ugly

@a-parser
Copy link
Author

related #30709

@bnoordhuis
Copy link
Member

Math is JavaScript builtin object, seems it not need to be proxied within vm module

That's how V8's API works, it's all or nothing. If you feel the documentation is lacking, go ahead and send a pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants