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: build v8 with -fvisibility=hidden -fvisibility-inlines-hidden #56290

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joyeecheung
Copy link
Member

Split from #56275 since it seems to cause gcc on some machines in the CI to time out or run out of memory. Trying to see if it's just a CI hiccup or if it's something that needs to be worked around.

@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 17, 2024
@joyeecheung joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed v8 engine Issues and PRs related to the V8 dependency. tools Issues and PRs related to the tools directory. needs-ci PRs that need a full CI run. labels 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

@richardlau
Copy link
Member

FWIW changing compile/link options will invalidate any stored CCACHE entries, which would result in more of V8 having to be recompiled.

Generally most of the x64 CI machines in Jenkins are now 4 GB RAM + swap (2 GB).

Anything running on a machine with _container_ in its name will be sharing memory with other containers on the same host (albeit these hosts have a lot of memory (32 GB, IIRC)).

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 9, 2025

In case anyone is curious, here is a bar chart I generated with ChatGPT about the busiest hours of the CI, using data from https://ci.nodejs.org/job/node-test-pull-request/api/json?tree=builds[timestamp,url,result] (past 100 jobs)

output (1)

@bnoordhuis
Copy link
Member

Dem 8 hour workday Europeans kicking off a quick CI run right before dinner, huh? That's a cool graph!

@joyeecheung joyeecheung force-pushed the v8-visibility-cflags branch from 3752a7e to ad5b353 Compare January 9, 2025 23:34
@joyeecheung
Copy link
Member Author

(Unless you live in Spain, where 8PM is considered early for dinner).

In seriousness I think the spike is probably due to being an overlap of working hours between Europe and Americas, in any case 16-18 CET would be the time to avoid testing something that invalidates the ccache. I dumped the script to https://gist.github.com/joyeecheung/6006b7ee781a2a619021767e0c8f2c01 though I have no idea if it works locally.

Rebased and added -fvisibility-inlines-hidden. It seems the Node.js files are missing the flags too, though I am not yet sure if all the other headers are set up correctly for us to apply the flags directly without breaking addons, so I've commented it out.

@joyeecheung joyeecheung marked this pull request as ready for review January 10, 2025 00:11
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2025
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 10, 2025

Started a CI, though I noticed that test-orka-macos11-x64-1 had been offline for a couple of days, creating a very long osx build queue, I just cleaned it up and brought it online, so not sure how well it would fare after this CI gets its turn in the osx jobs. If it doesn't look great in the build load, then not adding -fvisibility-inlines-hidden on macOS for now would be a better idea otherwise it would make the osx build queue even longer

@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 10, 2025

Looks like the build is still timing out on some platforms, need to try again in a less busy hour...

There was an error on AIX that doesn't show up on other platforms https://ci.nodejs.org/job/node-test-commit-aix/55380/nodes=aix72-ppc64/testReport/junit/addons/heap-profiler/test/

duration_ms: 142.114
exitcode: 1
severity: fail
stack: "node:internal/modules/cjs/loader:1930\n  return process.dlopen(module, path.toNamespacedPath(filename));\n\
  \                 ^\n\nError: rtld: 0712-001 Symbol _ZN2v812OutputStream12GetChunkSizeEv\
  \ was referenced\n      from module /home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/addons/heap-profiler/build/Release/binding.node(),\
  \ but a runtime definition\n\t    of the symbol was not found.\nrtld: 0712-001 Symbol\
  \ _ZN2v812OutputStream19WriteHeapStatsChunkEPNS_15HeapStatsUpdateEi was referenced\n\
  \      from module /home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/addons/heap-profiler/build/Release/binding.node(),\
  \ but a runtime definition\n\t    of the symbol was not found.\n    at Object..node\
  \ (node:internal/modules/cjs/loader:1930:18)\n    at Module.load (node:internal/modules/cjs/loader:1473:32)\n\
  \    at Function._load (node:internal/modules/cjs/loader:1285:12)\n    at TracingChannel.traceSync\
  \ (node:diagnostics_channel:322:14)\n    at wrapModuleLoad (node:internal/modules/cjs/loader:234:24)\n\
  \    at Module.require (node:internal/modules/cjs/loader:1495:12)\n    at require\
  \ (node:internal/modules/helpers:135:16)\n    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/addons/heap-profiler/test.js:5:17)\n\
  \    at Module._compile (node:internal/modules/cjs/loader:1739:14)\n    at Object..js\
  \ (node:internal/modules/cjs/loader:1904:10) {\n  code: 'ERR_DLOPEN_FAILED'\n}\n\
  \nNode.js v24.0.0-pre"

AFAICT, it shouldn't be failing as v8::OutputStream is marked as V8_EXPORT and it works fine on other platforms (the test also seemed to work fine from the CI started 3 weeks ago). Pinging @nodejs/platform-aix to see what's going on, if there's no one who cares about binary size on AIX to fix it I will just skip the settings on AIX I guess.

@richardlau
Copy link
Member

There was an error on AIX that doesn't show up on other platforms https://ci.nodejs.org/job/node-test-commit-aix/55380/nodes=aix72-ppc64/testReport/junit/addons/heap-profiler/test/

AFAICT, it shouldn't be failing as v8::OutputStream is marked as V8_EXPORT and it works fine on other platforms (the test also seemed to work fine from the CI started 3 weeks ago). Pinging @nodejs/platform-aix to see what's going on, if there's no one who cares about binary size on AIX to fix it I will just skip the settings on AIX I guess.

Seems strange that it worked 3 weeks ago but not in the recent run.

Symbol exporting is different on AIX -- we have https://github.com/nodejs/node/blob/main/tools/create_expfile.sh to generate an exports file containing the symbols that is used for addons:

# Writes out all of the exported symbols to a file.
# This is needed on AIX as symbols are not exported
# by an executable by default and need to be listed
# specifically for export so that they can be used
# by native add-ons.

We haven't touched it in five years -- I'm not even sure it's needed anymore since https://gcc.gnu.org/gcc-7/changes.html has

Visibility support has been enabled for AIX 7.1 and above.

@abmusse Is this something you can look into?

@joyeecheung joyeecheung changed the title build: build v8 with -fvisibility=hidden on non-macOS platforms too build: build v8 with -fvisibility=hidden -fvisibility-inlines-hidden Jan 10, 2025
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.

4 participants