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

build: arm64 cross-compile time improvement #45432

Merged
merged 1 commit into from
Nov 18, 2022

Conversation

StefanStojanovic
Copy link
Contributor

It can be observed on Jenkins, that Windows ARM64 build times of Node v14 and the latest version differ very much (v14 is built 3 times faster than the current version). After bisecting commits, I've found out that this change introduced that compilation time increment. I've reverted this change and tested cross-compilation locally and I didn't encounter any problems. As a result, I'm opening this PR to see if the results will be the same in the CI.

@nsait-linaro @targos I'm tagging the two of you for visibility because you've opened the original PR and approved it respectively.

Refs: #42538
Refs: #42375

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Nov 11, 2022
@lpinca
Copy link
Member

lpinca commented Nov 11, 2022

It seems this is a total revert. If so, can you please use git revert and the "Revert ..." commit message?

@niyas-sait
Copy link
Contributor

LGTM

lib/internal/encoding.js Outdated Show resolved Hide resolved
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Dismissing previous review.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022
Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

LGTM!

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

…st builds"

This reverts commit 818284b.

Reverted commit is a no longer needed patch for ARM64 cross-compiling.
It was increasing cross-compile time drastically (~3 times longer).
@StefanStojanovic
Copy link
Contributor Author

It seems this is a total revert. If so, can you please use git revert and the "Revert ..." commit message?

@lpinca I've missed your comment previously. Anyway, you are correct it is a total revert and I made changes to my commit as suggested.

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 14, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 14, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@StefanStojanovic
Copy link
Contributor Author

These CI errors seem unrelated to my change. Is there something I can do to help move this forward?

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 18, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 18, 2022
@nodejs-github-bot nodejs-github-bot merged commit 1f63c95 into nodejs:main Nov 18, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 1f63c95

ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
…st builds"

This reverts commit 818284b.

Reverted commit is a no longer needed patch for ARM64 cross-compiling.
It was increasing cross-compile time drastically (~3 times longer).

PR-URL: #45432
Refs: #42538
Refs: #42375
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Nov 23, 2022
…st builds"

This reverts commit 818284b.

Reverted commit is a no longer needed patch for ARM64 cross-compiling.
It was increasing cross-compile time drastically (~3 times longer).

PR-URL: nodejs#45432
Refs: nodejs#42538
Refs: nodejs#42375
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
…st builds"

This reverts commit 818284b.

Reverted commit is a no longer needed patch for ARM64 cross-compiling.
It was increasing cross-compile time drastically (~3 times longer).

PR-URL: #45432
Refs: #42538
Refs: #42375
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
…st builds"

This reverts commit 818284b.

Reverted commit is a no longer needed patch for ARM64 cross-compiling.
It was increasing cross-compile time drastically (~3 times longer).

PR-URL: #45432
Refs: #42538
Refs: #42375
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
…st builds"

This reverts commit 818284b.

Reverted commit is a no longer needed patch for ARM64 cross-compiling.
It was increasing cross-compile time drastically (~3 times longer).

PR-URL: #45432
Refs: #42538
Refs: #42375
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
…st builds"

This reverts commit 818284b.

Reverted commit is a no longer needed patch for ARM64 cross-compiling.
It was increasing cross-compile time drastically (~3 times longer).

PR-URL: #45432
Refs: #42538
Refs: #42375
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
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. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants