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

Revert "Add support for Bun (#20)" #21

Closed
wants to merge 1 commit into from

Conversation

AaronDewes
Copy link

This reverts commit 5bb4f50.

The latest Bun works with this package again (oven-sh/bun#5802).

@sindresorhus
Copy link
Owner

Is there any reason to revert though? The current version works too and is less fragile.

@AaronDewes
Copy link
Author

AaronDewes commented Sep 30, 2023

I thought the original change would make performance worse, so I did some benchmarks...

function callsites_current() {
        const _prepareStackTrace = Error.prepareStackTrace;
        try {
                let result = [];
                Error.prepareStackTrace = (_, callSites) => {
                        const callSitesWithoutCurrent = callSites.slice(1);
                        result = callSitesWithoutCurrent;
                        return callSitesWithoutCurrent;
                };

                new Error().stack; // eslint-disable-line unicorn/error-message, no-unused-expressions
                return result;
        } finally {
                Error.prepareStackTrace = _prepareStackTrace;
        }
}

function callsites_original() {
        const _prepareStackTrace = Error.prepareStackTrace;
        Error.prepareStackTrace = (_, stack) => stack;
        const stack = new Error().stack.slice(1); // eslint-disable-line unicorn/error-message
        Error.prepareStackTrace = _prepareStackTrace;
        return stack;
}


function callsites_with_try_finally() {
        const _prepareStackTrace = Error.prepareStackTrace;
        try {
            const stack = new Error().stack.slice(1);
                return stack;
        } finally {
                Error.prepareStackTrace = _prepareStackTrace;
        }
}

function callsites_with_try_catch() {
        const _prepareStackTrace = Error.prepareStackTrace;
        try {
            const stack = new Error().stack.slice(1);
                Error.prepareStackTrace = _prepareStackTrace;
                return stack;
        } catch {
                Error.prepareStackTrace = _prepareStackTrace;
        }
}



console.time('Original callsites (pre-bun)');
for (let i = 0; i < 100000; i++) {
  callsites_original();
}
console.timeEnd('Original callsites (pre-bun)');

console.time('Callsites with Bun');
for (let i = 0; i < 100000; i++) {
  callsites_current();
}
console.timeEnd('Callsites with Bun');

console.time('Callsites with try-finally block');
for (let i = 0; i < 100000; i++) {
  callsites_with_try_finally();
}
console.timeEnd('Callsites with try-finally block');


console.time('Callsites with try-catch block');
for (let i = 0; i < 100000; i++) {
  callsites_with_try_finally();
}
console.timeEnd('Callsites with try-catch block');

In Node:

Original callsites (pre-bun): 371.756ms
Callsites with Bun: 391.198ms
Callsites with try-finally block: 1.095s
Callsites with try-catch block: 1.146s

In Bun:

[122.09ms] Original callsites (pre-bun)
[112.69ms] Callsites with Bun
[100.88ms] Callsites with try-finally block
[99.62ms] Callsites with try-catch block

I have no idea why the "cleaner" implementation is so much worse in Node 🤔

In Deno, it's the same...:

Original callsites (pre-bun): 225ms
Callsites with Bun: 234ms
Callsites with try-finally block: 910ms
Callsites with try-catch block: 929ms

But in Chrome, this does not happen:

Original callsites (pre-bun): 160.52001953125 ms
Callsites with Bun: 172.321044921875 ms
Callsites with try-finally block: 172.614990234375 ms
Callsites with try-catch block: 181.050048828125 ms

So I wonder whether maybe V8 recently optimized performance for that and Chrome has that improvement, but Node & Deno don't 🤷‍♂️

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.

2 participants