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.

Sign up for GitHub

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

Jump to bottom

build: build v8 with -fvisibility=hidden on macOS #56275

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

build: build v8 with -fvisibility=hidden on macOS #56275

wants to merge 2 commits into from
+13 −0

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Dec 16, 2024 •
edited
Loading

V8 should be built with -fvisibility=hidden, otherwise the resulting binary would contain unnecessary symbols. In particular, on macOS, this leads to 5000+ weak symbols resolved at runtime, leading to a startup regression.

On macOS this also reduces the binary size about ~10MB. On Linux the size reduction is around 8MB.

$ DYLD_PRINT_BINDINGS=1 ./node-c4aa34aa --version 2>&1 | grep "looking for weak-def symbol" | wc -l
    7317
$ DYLD_PRINT_BINDINGS=1 out/Release/node --version 2>&1 | grep "looking for weak-def symbol" | wc -l
    1755

$ hyperfine "./node-c4aa34aa --version" "out/Release/node --version" --warmup 10
Benchmark 1: ./node-c4aa34aa --version
  Time (mean ± σ):      28.9 ms ±   1.0 ms    [User: 26.5 ms, System: 1.9 ms]
  Range (min … max):    28.1 ms …  32.2 ms    89 runs

Benchmark 2: out/Release/node --version
  Time (mean ± σ):      12.4 ms ±   1.0 ms    [User: 9.1 ms, System: 1.9 ms]
  Range (min … max):    11.6 ms …  16.2 ms    148 runs

Summary
  'out/Release/node --version' ran
    2.33 ± 0.20 times faster than './node-c4aa34aa --version'

$ ls -lah out/Release/node
-rwxr-xr-x  1 joyee  staff   108M Dec 16 22:39 out/Release/node
$ ls -lah ./node-c4aa34aa
-rwxr-xr-x  1 joyee  staff   118M Dec  5 12:39 ./node-c4aa34aa

On Linux the performance is mostly the same as Linux's ld doesn't resolve the weak symbols like macOS's dyld does in the first place, so there was no regression to fix. This does however makes the binary a bit smaller. EDIT: split out the non-macOS settings to #56290 since it seems to cause gcc to time out or run out of memory on some machines in the CI, possibly because it invalidates the ccache on too many machines.

$ hyperfine "./node-hidden ./test/fixtures/empty.js" "./node-main ./test/fixtures/empty.js" --warmup=10
Benchmark 1: ./node-hidden ./test/fixtures/empty.js
  Time (mean ± σ):      18.0 ms ±   0.3 ms    [User: 10.0 ms, System: 8.4 ms]
  Range (min … max):    17.2 ms …  18.7 ms    165 runs

Benchmark 2: ./node-main ./test/fixtures/empty.js
  Time (mean ± σ):      18.4 ms ±   0.3 ms    [User: 9.8 ms, System: 8.9 ms]
  Range (min … max):    17.7 ms …  19.1 ms    158 runs

Summary
  './node-hidden ./test/fixtures/empty.js' ran
    1.02 ± 0.02 times faster than './node-main ./test/fixtures/empty.js'

$ ls -lah ./node-main ./node-hidden
-rwxrwxr-x 1 ubuntu ubuntu 113M Dec 16 23:05 ./node-hidden
-rwxrwxr-x 1 ubuntu ubuntu 121M Dec 16 23:59 ./node-main

Fixes: nodejs/performance#180

@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added 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 Dec 16, 2024
juanarbol
juanarbol approved these changes Dec 16, 2024
View reviewed changes
Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

Impressive catch, thank you.

Rubber stamp LGTM

@joyeecheung joyeecheung mentioned this pull request Dec 16, 2024
Open
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/64071/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6324/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6324/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6324/

@lemire
Copy link
Member

lemire commented Dec 16, 2024

Looks like great work! Congratulations!

@joyeecheung joyeecheung changed the title build: build v8 with -fvisibility=hidden on macOS build: build v8 with -fvisibility=hidden Dec 17, 2024
@joyeecheung
Copy link
Member Author

Updated to also apply the flag to Linux. The performance profile on Linux stays the same since Linux didn't have that regression in the first place, though this does shrink the binary by about 8MB.

@joyeecheung
Copy link
Member Author

joyeecheung commented Dec 17, 2024 •
edited
Loading

Added BUILDING_V8_SHARED to fix the addon tests since we'll now need V8_EXPORT to explicitly mark the symbols as visible.

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2024
@lemire
Copy link
Member

lemire commented Dec 17, 2024

Someone should give Joyee an award for this PR.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/64075/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6325/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6325/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6325/

@anonrig
Copy link
Member

anonrig commented Dec 17, 2024

I want to approve this PR multiple times just to show my respect.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/64080/

mcollina
mcollina approved these changes Dec 17, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@joyeecheung
build: build v8 with -fvisibility=hidden on macOS
3b1bdd4 
V8 should be built with -fvisibility=hidden, otherwise
the resulting binary would contain unnecessary symbols. In
particular, on macOS, this leads to 5000+ weak symbols
resolved at runtime, leading to a startup regression.

On macOS this also reduces the binary size about ~10MB.

It's only enabled on macOS in this patch as gcc can time out
or run out of memory on some machines in the CI with
-fvisibility=hidden.
@joyeecheung
Copy link
Member Author

It seems on some Linux configs and on SmartOS, with -fvisibility=hidden gcc is either running out of memory or timing out especially when building turboshaft (my guess is that turning this on might give gcc too much room to optimize which requires resources that not all machines in our CI currently have). I will split out the cflags setting for non-macOS platforms to another PR, and let this PR focus on macOS since that's the only platform where not fixing this leads to a startup regression.

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2024
@joyeecheung joyeecheung changed the title build: build v8 with -fvisibility=hidden build: build v8 with -fvisibility=hidden on macOS Dec 17, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/64083/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6327/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6327/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6327/

@joyeecheung joyeecheung mentioned this pull request Dec 17, 2024
Draft
legendecas
legendecas approved these changes Dec 17, 2024
View reviewed changes
tools/v8_gypfiles/v8.gyp Outdated
'GCC_SYMBOLS_PRIVATE_EXTERN': 'YES', # -fvisibility=hidden
},
'defines': [
'BUILDING_V8_SHARED', # Make V8_EXPORT visible.
Copy link
Member

Choose a reason for hiding this comment

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

node/common.gypi

Lines 478 to 479 in 5a868b5

'BUILDING_V8_SHARED=1',
'BUILDING_UV_SHARED=1',
non-blocking: common.gypi probably needs to be consolidated

Copy link
Member Author

@joyeecheung joyeecheung Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think for addons, common.gypi should define USING_V8_SHARED instead, then in node.gypi we need to undefine it/override it to BUILDING_V8_SHARED when building libnode, though that might be a bit challenging to get right with the messy gyp config inheritance all over the place..for now there should probably not even be any difference on Windows (USING_V8_SHARED only affects Windows AFAICT, but currently both the addons and the Node.js build have BUILDING_V8_SHARED on Windows before and after this PR, even though it's technically not quite right for the addons to do that - not that it's going to be buggy, just less optimized). We can look into it in a follow up.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, agreed that addons should have USING_V8_SHARED defined. Right now, it is defined in node-gyp. So here in common.gypi, it may be removed in a follow-up.

https://github.com/nodejs/node-gyp/blob/b899faed56270d3d8496da7576b5750b264c2c21/addon.gypi#L36

@joyeecheung
Copy link
Member Author

joyeecheung commented Dec 17, 2024 •
edited
Loading

It seems non-macOS platforms are still timing out, likely due to ccache invalidation (#56290 (comment))? So I changed the config to completely scoped to macOS in this PR - for non-macOS updates I will find a time when the CI is less busy to work on it in #56290

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/64091/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6328/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6328/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6328/

@ryanaslett ryanaslett mentioned this pull request Dec 17, 2024
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legendecas legendecas legendecas approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@juanarbol juanarbol juanarbol approved these changes

@targos targos targos approved these changes

@anonrig anonrig anonrig approved these changes

@lpinca lpinca lpinca approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@lemire lemire lemire approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Potential startup regression since on Node.js >= 22