Skip to content

feat(bench): Add @napi-rs/simple-git to benchmarks#76

Merged
seokju-na merged 5 commits intotoss:mainfrom
nnnnoel:feat/simple-git-benchmarks
Apr 7, 2025
Merged

feat(bench): Add @napi-rs/simple-git to benchmarks#76
seokju-na merged 5 commits intotoss:mainfrom
nnnnoel:feat/simple-git-benchmarks

Conversation

@nnnnoel
Copy link
Contributor

@nnnnoel nnnnoel commented Mar 27, 2025

I added @napi-rs/simple-git benchmark for test performance

NOTE

Need to update benchmark results with maintainer's laptop.

@vercel
Copy link

vercel bot commented Mar 27, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
es-git ⬜️ Skipped (Inspect) Apr 7, 2025 2:43am

"dependencies": {
"nodegit": "link:./nodegit"
"nodegit": "link:./nodegit",
"simple-git": "link:./simple-git"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since, @napi-rs/simple-git not uses node-gyp and includes prebuilt across all platforms, how about installing it directly with the yarn workspace module?

Suggested change
"simple-git": "link:./simple-git"
"@napi-rs/simple-git": "0.1.19"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I thought it's conversion of benchmark folder.
I'll change it when I possible to work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since nodegit does not have a prebuilt for workspaces Node.js version, so installation with the yarn workspace module may be failed depending on the users environment. Therefore, we are safely separating the package and installing it manually. 😭
For detailed instructions, please refer to benchmarks/README.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seokju-na I updated commit for this review

await NodeGitRepository.open(gitDir);
});

bench('simple-git', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bench('simple-git', () => {
bench('@napi-rs/simple-git', () => {

Could you please be more specific about the name, as it may overlap with other packages? (e.g. https://github.com/steveukx/git-js/tree/main/simple-git)

@seokju-na
Copy link
Collaborator

Greate jobs. Thanks for your contribution!

@nnnnoel nnnnoel requested a review from seokju-na March 31, 2025 01:36
@seokju-na
Copy link
Collaborator

Could you please check for CI failures?
I will update the benchmark after this PR is merged.

Copy link
Collaborator

@seokju-na seokju-na left a comment

Choose a reason for hiding this comment

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

👍

@seokju-na seokju-na merged commit 25bb21b into toss:main Apr 7, 2025
27 checks passed
other-yuka added a commit to other-yuka/es-git that referenced this pull request Apr 8, 2025
* feat(bench): Add `@napi-rs/simple-git` to benchmarks (toss#76)

Co-authored-by: seokju-na <seokju.me@toss.im>

* chore(docs): fix some typos

* test

---------

Co-authored-by: Noel Kim (김민혁) <noelkim@prestolabs.io>
Co-authored-by: seokju-na <seokju.me@toss.im>
@seokju-na
Copy link
Collaborator

Benchmark updated from #101
Checkout the performance page in documentation!

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.

3 participants