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: revert conversion of 'debug' package for 'node:util::debuglog' #173

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

jeremy-daley-kr
Copy link
Contributor

Description

A major performance regression was detected amounting to about 10x speed decrease. This occurred by the changing of how router is debugged.

Please see the following discussion:
koajs/koa#1781

Checklist

  • I have ensured my pull request is not behind the main or master branch of the original repository.
  • I have rebased all commits where necessary so that reviewing this pull request can be done without having to merge it first.
  • I have written a commit message that passes commitlint linting.
  • I have ensured that my code changes pass linting tests.
  • I have ensured that my code changes pass unit tests.
  • I have described my pull request and the reasons for code changes along with context if necessary.

@jeremy-daley-kr
Copy link
Contributor Author

@titanism Can you please look into what's failing here?

I don't believe it's from my PR, as I can see the same error in your commit from yesterday:
8d390a9

@titanism titanism merged commit 04884de into koajs:master Oct 12, 2023
1 of 4 checks passed
@titanism
Copy link
Contributor

@jeremy-daley-kr can you file a GitHub issue in Node.js that asks why debuglog is so slow or something? Probably something event loop related?

@titanism
Copy link
Contributor

Publishing v12.0.1 to npm now, almost done.

@titanism
Copy link
Contributor

titanism commented Oct 12, 2023

I'll most likely backport to v11 as patch too.

@jeremy-daley-kr jeremy-daley-kr deleted the fix/revert-debuglog branch October 12, 2023 20:18
@jeremy-daley-kr
Copy link
Contributor Author

@jeremy-daley-kr can you file a GitHub issue in Node.js that asks why debuglog is so slow or something? Probably something event loop related?

I'll see what I can do. Thanks for moving quickly with the merge. I'll keep my eye out for 12.0.1 👀

@titanism
Copy link
Contributor

@titanism
Copy link
Contributor

Deprecation notice published for all versions of koa-router and @koa/router (mirrored packages) of **IMPORTANT 10x+ PERFORMANCE UPGRADE**: Please upgrade to v12.0.1+ as we have fixed an issue with debuglog causing 10x slower router benchmark performance, see https://github.com/koajs/router/pull/173.

@titanism
Copy link
Contributor

Manually backported and released as v11.0.1 for koa-router and @koa/router.

@titanism
Copy link
Contributor

All done! 🎉 thanks again @jeremy-daley-kr - if you want some free credit for Forward Email just ping us at support@forwardemail.net once you sign up. 🖖

Copy link

@MoFaro MoFaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello i'm a beginner here, i have this error when i try to create my strapi project i have seen all these but i don't know how to process. I really need your help.
here is one of the errors:

@strapi/strapi > @koa/router@10.1.1: IMPORTANT 10x+ PERFORMANCE UPGRADE: Please upgrade to v12.0.1+ as we have fixed an issue with debuglog causing 10x slower router benchmark performance, see #173.

@titanism
Copy link
Contributor

@MoFaro please read the deprecation and the issue. this is a warning to upgrade, not an error. it is a performance-related issue.

@4nduril-ms
Copy link

Sorry to bump that one. But is it intended that version 11.0.2 has the "latest" tag instead of 12.0.1?

@titanism
Copy link
Contributor

@4nduril-ms amazing catch, I didn't specify --tag, fixing now, 🙏 - v12.0.2 releasing now

@titanism
Copy link
Contributor

@4nduril-ms I actually didn't need to do another package version bump, there's npm dist-tag which was run successfully ✅

❯ npm dist-tag add @koa/router@12.0.1 latest
Authenticate your account at:
https://www.npmjs.com/auth/cli/xxx
Press ENTER to open in the browser...

+latest: @koa/router@12.0.1
❯ npm dist-tag add koa-router@12.0.1 latest
Authenticate your account at:
https://www.npmjs.com/auth/cli/xxx
Press ENTER to open in the browser...

+latest: koa-router@12.0.1

thanks again 🙏

@4nduril-ms
Copy link

@titanism Great! Thank you for fixing it that fast.

@titanism
Copy link
Contributor

@4nduril-ms of course, also if you need anything email related, check out our project https://forwardemail.net 🔥

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.

4 participants