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

chore: improve perf of hot path #637

Merged
merged 2 commits into from Nov 21, 2023
Merged

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Nov 21, 2023

Reduce the hot path in core ;)

An investigation regarding the hotpaths showed that the console.warn and .error binding results take some time. Also a forEach call is Kind of hot.

To improve the performance console.warn and console.error are bound once at startup.
before
Screenshot from 2023-11-20 23-11-43

Resolves #ISSUE_NUMBER


Before the change?

After the change?

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 21, 2023

@wolfy1339
@gr2m

@wolfy1339
Copy link
Member

Can you describe the fix and the problem with text instead of just a picture next time.

It would help with searchability and context in the future if this comes back up

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 21, 2023

@wolfy1339 added some context

@wolfy1339 wolfy1339 merged commit 8e5d7c1 into octokit:main Nov 21, 2023
7 checks passed
@Uzlopak Uzlopak deleted the improve-on-hot-path branch November 21, 2023 17:08
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

a chore: commit will not trigger a new release. Improving performance without changing behavior is a fix:

console.warn = originalLogWarn;
console.error = originalLogError;

expect(calls).toStrictEqual(["warn", "error"]);
Copy link
Contributor

Choose a reason for hiding this comment

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

you changed the nature of this test, can you please revert that? Please add a test where we log with all 4 methods

 octokit.log.debug("foo");
    octokit.log.info("bar");
    octokit.log.warn("baz");
    octokit.log.error("daz");

and check that only warn and error was logged

    expect(calls).toStrictEqual(["warn", "error"]);

wolfy1339 pushed a commit that referenced this pull request Nov 22, 2023
Copy link
Contributor

🎉 This PR is included in version 5.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

None yet

3 participants