Skip to content

Conversation

@kvakil
Copy link
Contributor

@kvakil kvakil commented Jan 4, 2024

Enable V8's new maglev compiler by default on supported architectures.
This brings modest performance improvements for short-lived workloads
like CLI programs (see the linked issue) and brings Node.js's
configuration slightly closer to Google Chrome's.

I marked this change as semver-major because Maglev can theoretically
cause performance regressions, although I haven't seen an example of
that in the (somewhat limited) benchmarking I've done.

Notable Change Summary: V8's Maglev Compiler is now enabled by
default on supported architectures (https://v8.dev/blog/maglev). Maglev
improves CPU performance for short-lived CLI programs by around 8%.

Fixes: #50690

Enable V8's new maglev compiler by default on supported architectures.
This brings modest performance improvements for short-lived workloads
like CLI programs (see the linked issue) and brings Node.js's
configuration slightly closer to Google Chrome's.

I marked this change as semver-major because Maglev can theoretically
cause performance regressions, although I haven't seen an example of
that in the (somewhat limited) benchmarking I've done.

**Notable Change Summary:** V8's Maglev Compiler is now enabled by
default on supported architectures (https://v8.dev/blog/maglev). Maglev
improves CPU performance for short-lived CLI programs by around 8%.

Fixes: nodejs#50690
@kvakil kvakil added the v8 engine Issues and PRs related to the V8 dependency. label Jan 4, 2024
@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. labels Jan 4, 2024
@kvakil kvakil added semver-major PRs that contain breaking changes and should be released in the next major version. notable-change PRs with changes that should be highlighted in changelogs. build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. and removed build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Jan 4, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2024

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @kvakil.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

@kvakil kvakil added request-ci Add this label to start a Jenkins CI on a PR. performance Issues and PRs related to the performance of Node.js. labels Jan 4, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2024
@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Jan 4, 2024

@nodejs/python

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Jan 4, 2024

@kvakil
Copy link
Contributor Author

kvakil commented Jan 4, 2024

Breaks ARM cross-compilation: https://ci.nodejs.org/job/node-cross-compile/46243/nodes=cross-compiler-rhel8-armv7-gcc-10-glibc-2.28/

It looks like the failing part is:

00:34:34   ccache /opt/raspberrypi/rpi-newer-crosstools/x64-gcc-10.3.0-glibc-2.28/arm-rpi-linux-gnueabihf/bin/arm-rpi-linux-gnueabihf-g++ -march=armv7-a -o /home/iojs/build/workspace/node-cross-compile/out/Release/obj.target/v8_compiler/deps/v8/src/compiler/backend/move-optimizer.o ../deps/v8/src/compiler/backend/move-optimizer.cc '-D_GLIBCXX_USE_CXX11_ABI=1' '-DNODE_OPENSSL_CONF_NAME=nodejs_conf' '-DNODE_OPENSSL_HAS_QUIC' '-DICU_NO_USER_DATA_OVERRIDE' '-DV8_GYP_BUILD' '-DV8_TYPED_ARRAY_MAX_SIZE_IN_HEAP=64' '-D__STDC_FORMAT_MACROS' '-DOPENSSL_NO_PINSHARED' '-DOPENSSL_THREADS' '-DV8_TARGET_ARCH_ARM' '-DCAN_USE_ARMV7_INSTRUCTIONS' '-DCAN_USE_VFP3_INSTRUCTIONS' '-DCAN_USE_VFP32DREGS' '-DV8_HAVE_TARGET_OS' '-DV8_TARGET_OS_LINUX' '-DV8_EMBEDDER_STRING="-node.19"' '-DENABLE_DISASSEMBLER' '-DV8_PROMISE_INTERNAL_FIELD_COUNT=1' '-DV8_ENABLE_PRIVATE_MAPPING_FORK_OPTIMIZATION' '-DOBJECT_PRINT' '-DV8_INTL_SUPPORT' '-DV8_ATOMIC_OBJECT_FIELD_WRITES' '-DV8_ENABLE_LAZY_SOURCE_POSITIONS' '-DV8_USE_SIPHASH' '-DV8_SHARED_RO_HEAP' '-DV8_WIN64_UNWINDING_INFO' '-DV8_ENABLE_REGEXP_INTERPRETER_THREADED_DISPATCH' '-DV8_USE_ZLIB' '-DV8_ENABLE_MAGLEV' '-DV8_ENABLE_TURBOFAN' '-DV8_ENABLE_WEBASSEMBLY' '-DV8_ENABLE_JAVASCRIPT_PROMISE_HOOKS' '-DV8_ALLOCATION_FOLDING' '-DV8_ALLOCATION_SITE_TRACKING' '-DV8_ADVANCED_BIGINT_ALGORITHMS' '-DUCONFIG_NO_SERVICE=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=1' '-DUCONFIG_NO_BREAK_ITERATION=0' -I../deps/v8 -I../deps/v8/include -I/home/iojs/build/workspace/node-cross-compile/out/Release/obj/gen/generate-bytecode-output-root -I/home/iojs/build/workspace/node-cross-compile/out/Release/obj/gen -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common  -pthread -Wno-unused-parameter -Wno-return-type -flax-vector-conversions -Wno-invalid-offsetof -fno-strict-aliasing -march=armv7-a -mfpu=vfpv3 -mfloat-abi=hard -marm -O3 -fno-omit-frame-pointer -fdata-sections -ffunction-sections -O3 -fno-rtti -fno-exceptions -std=gnu++17 -MMD -MF /home/iojs/build/workspace/node-cross-compile/out/Release/.deps//home/iojs/build/workspace/node-cross-compile/out/Release/obj.target/v8_compiler/deps/v8/src/compiler/backend/move-optimizer.o.d.raw   -c
00:36:31 g++: fatal error: Killed signal terminated program cc1plus

It's a little difficult to tell what got killed here, but it looks like the Makefile is running without parallelism, so I'm assuming it's the compilation of deps/v8/src/compiler/backend/move-optimizer.cc. That isn't related to Maglev, so I suspect something else.

I looked through the other CI failures and they look unrelated, except potentially the Ubuntu arm64 debug build which has several suspicious looking segfaults.

Anyway, I'm going to resume the CI job.

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

Can we have a benchmark script or something added to nodejs internal benchmark to measure the impact of this new compiler?

@kvakil
Copy link
Contributor Author

kvakil commented Jan 4, 2024

Can we have a benchmark script or something added to nodejs internal benchmark to measure the impact of this new compiler?

Hm, that seems like a big lift. In my mind V8 is responsible for "general Javascript performance" and Node.js is responsible for "standard library performance". I'm not sure if trying to create a representative general Javascript performance benchmark in Node.js would be possible, but maybe I'm misunderstanding your request.

If you're looking for some ad-hoc benchmarking, there's some of that in the linked issue.

@merceyz
Copy link
Member

merceyz commented Jan 5, 2024

Perhaps the benchmarks added in #50684 would do?

@kvakil
Copy link
Contributor Author

kvakil commented Jan 5, 2024

Perhaps the benchmarks added in #50684 would do?

Yes, it's a good idea. I tried that in #50690 (comment). --version doesn't actually do enough work to tier up into Maglev.

@kvakil kvakil added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 5, 2024
@RafaelGSS
Copy link
Member

Hm, that seems like a big lift. In my mind V8 is responsible for "general Javascript performance" and Node.js is responsible for "standard library performance". I'm not sure if trying to create a representative general Javascript performance benchmark in Node.js would be possible, but maybe I'm misunderstanding your request.

That works! I just wanted to check the impact of this PR even in micro-benchmarks. Just for comparison reasons.

@kvakil kvakil added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 8, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 8, 2024
@nodejs-github-bot nodejs-github-bot merged commit 1a5acd0 into nodejs:main Jan 8, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in 1a5acd0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. performance Issues and PRs related to the performance of Node.js. semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable Maglev

6 participants