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

fix: improve perf #444

Merged
merged 4 commits into from
Jul 11, 2024
Merged

fix: improve perf #444

merged 4 commits into from
Jul 11, 2024

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jul 11, 2024

If an Error is instantiated in nodejs, then the Error is enriched with the stacktrace anyway. I could not figure out, in which case the stacktrace could be wrong.

We significantly increase the instantiation speed.

The stacktrace generation is in general the slowest part of the error instantiation. So touching other parts of the code will not change the performance of the error instantiation significantly. E.g. we could check if the query contains a ? before doing the .replace calls for client_secret and access_token which should boost the performance, but it has no effect, because of the slow instantiation of Errors in general.

before:

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/request-error.js$ node ./benchmarks/instantiate.mjs 
┌─────────┬──────────────────────────────────────────────────────────────────┬──────────┬────────────────────┬──────────┬─────────┐
│ (index) │ Task Name                                                        │ ops/sec  │ Average Time (ns)  │ Margin   │ Samples │
├─────────┼──────────────────────────────────────────────────────────────────┼──────────┼────────────────────┼──────────┼─────────┤
│ 0       │ 'instantiate a simple RequestError''93,794' │ 10661.642643923351 │ '±0.82%' │ 9380    │
│ 1       │ 'instantiate a RequestError with authorization header in header''95,574' │ 10463.005231219588 │ '±0.79%' │ 9558    │
│ 2       │ 'instantiate a RequestError with client_secret in query''93,176' │ 10732.264971023862 │ '±1.28%' │ 9318    │
└─────────┴──────────────────────────────────────────────────────────────────┴──────────┴────────────────────┴──────────┴─────────┘

after:

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/request-error.js$ node ./benchmarks/instantiate.mjs 
┌─────────┬──────────────────────────────────────────────────────────────────┬───────────┬───────────────────┬──────────┬─────────┐
│ (index) │ Task Name                                                        │ ops/sec   │ Average Time (ns) │ Margin   │ Samples │
├─────────┼──────────────────────────────────────────────────────────────────┼───────────┼───────────────────┼──────────┼─────────┤
│ 0       │ 'instantiate a simple RequestError''163,904' │ 6101.095540235711 │ '±1.29%' │ 16391   │
│ 1       │ 'instantiate a RequestError with authorization header in header''158,418' │ 6312.399128897308 │ '±2.85%' │ 15842   │
│ 2       │ 'instantiate a RequestError with client_secret in query''160,375' │ 6235.384337199317 │ '±1.11%' │ 16038   │
└─────────┴──────────────────────────────────────────────────────────────────┴───────────┴───────────────────┴──────────┴─────────┘

Copy link

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@gr2m
Copy link
Contributor

gr2m commented Jul 11, 2024

I honestly don't recall why I added Error.captureStackTrace() to the code, I probably just copy&pasted it from some best practice page at the time, which was probably in 2018 or so.

I don't think performance of our error instantiation is a bottleneck, but I appreciate your thorough investigation and fix with receipts nonetheless

@gr2m gr2m merged commit ba04ffa into octokit:main Jul 11, 2024
6 checks passed
Copy link

🎉 This issue has been resolved in version 6.1.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Uzlopak Uzlopak deleted the improve-perf branch July 11, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants