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

doc: add mold to speeding up section #52179

Merged

Conversation

congzhangzh
Copy link
Contributor

C/C++ link is slow, especially for node like projects, which will link GBs data to 100MB like size
C/C++ link process can not be cached
mold is really really fast

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Mar 21, 2024
@congzhangzh
Copy link
Contributor Author

@H4ad Hi Vinicius, can you review this document improve?

@congzhangzh congzhangzh closed this by deleting the head repository Mar 22, 2024
@H4ad
Copy link
Member

H4ad commented Mar 22, 2024

@congzhangzh Sorry for the delay, this PR looks good to me, do you still want to close?

@sxa
Copy link
Member

sxa commented Mar 22, 2024

The first time I tried mold on my aarch64 system it crashed on me but it was ok on the second attempt , but assuming this was a one-off I reckon I'd be 👍🏻 on this suggestion to end users.

@congzhangzh congzhangzh reopened this Mar 22, 2024
@congzhangzh
Copy link
Contributor Author

@H4ad @sxa hi folks, please merge if you guys accept :)

The fork repo deleted unconditionally, and I restored it from GitHub support:)

@H4ad
Copy link
Member

H4ad commented Mar 23, 2024

Please, just fix the lint issues:

Using release list from env...
BUILDING.md is not formatted. Please run 'make format-md'.
BUILDING.md
             warning Remove trailing whitespace                    no-trailing-spaces  remark-lint
534:162      warning Line must be at most 80 characters            maximum-line-length remark-lint
537:84       warning Line must be at most 80 characters            maximum-line-length remark-lint
573:4-573:39 warning Don’t use literal URLs without angle brackets no-literal-urls     remark-lint
574:4-574:34 warning Don’t use literal URLs without angle brackets no-literal-urls     remark-lint

@congzhangzh
Copy link
Contributor Author

Please, just fix the lint issues:

Using release list from env...
BUILDING.md is not formatted. Please run 'make format-md'.
BUILDING.md
             warning Remove trailing whitespace                    no-trailing-spaces  remark-lint
534:162      warning Line must be at most 80 characters            maximum-line-length remark-lint
537:84       warning Line must be at most 80 characters            maximum-line-length remark-lint
573:4-573:39 warning Don’t use literal URLs without angle brackets no-literal-urls     remark-lint
574:4-574:34 warning Don’t use literal URLs without angle brackets no-literal-urls     remark-lint

@H4ad Hi Vinicius, I fixed it by make format-md :)

@congzhangzh
Copy link
Contributor Author

@H4ad @sxa @anonrig Hi folks, please review again, sorry for these repeated errors, I am a newbie to nodejs community:)

I will fix other document errors later, and I know I can do make lint-md and make format-md locally, that's very
helpful, cool :)

@H4ad H4ad added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Mar 25, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 25, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/52179
✔  Done loading data for nodejs/node/pull/52179
----------------------------------- PR info ------------------------------------
Title      doc: add mold to speeding up section (#52179)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     congzhangzh:add_mold_to_speed_up_section -> nodejs:main
Labels     doc, build, commit-queue-squash
Commits    3
 - doc: add mold to speeding up section
 - doc: format-md for speeding up section
 - doc: fix lint-md failed
Committers 2
 - GitHub 
 - Cong Zhang <13283869+congzhangzh@users.noreply.github.com>
PR-URL: https://github.com/nodejs/node/pull/52179
Reviewed-By: Vinícius Lourenço Claro Cardoso 
Reviewed-By: Yagiz Nizipli 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52179
Reviewed-By: Vinícius Lourenço Claro Cardoso 
Reviewed-By: Yagiz Nizipli 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - doc: fix lint-md failed
   ℹ  This PR was created on Thu, 21 Mar 2024 17:08:16 GMT
   ✔  Approvals: 2
   ✔  - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/52179#pullrequestreview-1956285442
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/52179#pullrequestreview-1956294857
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/8420226631

@congzhangzh
Copy link
Contributor Author

congzhangzh commented Mar 25, 2024

@H4ad Hi Vinicius, the last commit 4 fix lint-md need to be approved before landing, tks:)

@H4ad H4ad added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Mar 25, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 25, 2024
@nodejs-github-bot nodejs-github-bot merged commit fc02918 into nodejs:main Mar 25, 2024
24 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in fc02918

brew install ccache mold # see https://brew.sh
export CC="ccache cc" # add to ~/.zshrc or other shell config file
export CXX="ccache c++" # add to ~/.zshrc or other shell config file
export LDFLAGS="-fuse-ld=mold" # add to ~/.zshrc or other shell config file
Copy link
Member

Choose a reason for hiding this comment

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

Support for Mach-O targets has been removed.
See https://github.com/bluewhalesystems/sold for macOS/iOS support.

I think we should use sold in macOS

Copy link
Member

Choose a reason for hiding this comment

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

@congzhangzh Are you willing to open a PR to replace mold to sold on mac?

Copy link
Member

Choose a reason for hiding this comment

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

Don't forget that sold is not available on homebrew

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, please allow me do more test on mac os

@rui314 Hi Rui, is that ok we use sold in nodejs, as this mark on https://bluewhale.systems/ websit

What is the difference between sold and mold?
Other than the branding and their licenses, sold and mold behave the same. Both sold and mold are developed >> by us and our contributors.

btw, and whether mold merge all sold functions?

Copy link

Choose a reason for hiding this comment

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

sold is not actively maintained anymore because with Xcode 15, Apple has made their linker much faster than it was. So you want to recommend it instead of sold. IIRC, the Apple's new linker is available as ld-prime after installing Xcode 15.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sold is not actively maintained anymore because with Xcode 15, Apple has made their linker much faster than it was. So you want to recommend it instead of sold. IIRC, the Apple's new linker is available as ld-prime after installing Xcode 15.

@rui314 @H4ad @anonrig Hi folks, can you help to ensure that the new linker ld-prime works, which point by Rui

Copy link

Choose a reason for hiding this comment

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

Since Apple's linker is not my product, and I no longer working on macOS, I don't know whether that LDFLAGS works or not.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have macos :/

Also, as mentioned by @anonrig, I think we should remove mold from macos section and don't use sold since is not maintained anymore.

Copy link
Contributor Author

@congzhangzh congzhangzh Mar 28, 2024

Choose a reason for hiding this comment

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

ok, got it, I will do a pull request later:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have macos :/

Also, as mentioned by @anonrig, I think we should remove mold from macos section and don't use sold since is not maintained anymore.

@H4ad I create a merge request to fix it, #52252

anonrig pushed a commit to anonrig/node that referenced this pull request Mar 25, 2024
PR-URL: nodejs#52179
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#52179
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52179
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52179
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants