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

Enable Maglev #50690

Closed
6 tasks done
kvakil opened this issue Nov 12, 2023 · 11 comments · Fixed by #51360
Closed
6 tasks done

Enable Maglev #50690

kvakil opened this issue Nov 12, 2023 · 11 comments · Fixed by #51360
Labels
feature request Issues that request new features to be added to Node.js. performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency.

Comments

@kvakil
Copy link
Contributor

kvakil commented Nov 12, 2023

What is the problem this feature will solve?

V8 recently introduced their new Maglev compiler which is a mid-tier compiler (blog post, design doc).

We currently do not compile or ship Maglev.

What is the feature you are proposing to solve the problem?

Build and enable Maglev. Proposed plan:

  1. Get Maglev to actually compile with our settings.
  2. Run some benchmarks and post them here.
  3. Add a configuration option to compile with Maglev enabled.
  4. Upstream V8 patches to get Maglev compilation working.
  5. Cherry-pick upstream patches back to this repo.
  6. Flip the configuration flag to default-on and ship Maglev.

Open questions

  1. Do we want to ship this?
  2. Should turning the configuration flag to default-on be semver major? Or maybe it only needs to be dont-land-on-v20.x, and we can ship it as part of v21?

What alternatives have you considered?

We could also not enable Maglev, which is a valid choice.

Pros of shipping:

  • Will likely be faster?
  • Gets us closer to Google Chrome which ships Maglev.

Cons of shipping:

  • As a mid-tier compiler Maglev may not bring much performance benefit for long-running server-like workloads.
  • Enabling Maglev may cause performance regressions for users.
    • Users can disable Maglev with the V8 option --no-maglev but it may be difficult to figure out that Maglev is causing the regression.
  • Potentially leads to more maintenance burden from V8 changes.
@kvakil kvakil added v8 engine Issues and PRs related to the V8 dependency. feature request Issues that request new features to be added to Node.js. performance Issues and PRs related to the performance of Node.js. labels Nov 12, 2023
@kvakil
Copy link
Contributor Author

kvakil commented Nov 12, 2023

I ran a random subset of 82 benchmark files from the repo, once with maglev and once without. Overall the results from this sample look positive for Maglev, of the "statistically significant" changes:

  • assert/ suite generally positive.
  • async_hooks/ suite generally positive.
  • blob/ suite positive.
  • buffers/ suite positive.
  • events/ suite negative.
  • path/ suite positive.
  • streams/ suite generally positive.
  • url/ suite generally positive.
  • validators/ suite positive.

Here are the raw CSVs from benchmark/compare.js: results.csv.tar.gz, if you want to look at specific benchmarks. The benchmark suite is very slow, so I'm loathe to run the entire thing, but let me know if there are other benchmarks people would find informative.

@kvakil kvakil changed the title Maglev Enable Maglev Nov 12, 2023
kvakil added a commit to kvakil/node that referenced this issue Nov 13, 2023
This adds a configuration flag to enable V8's Maglev compiler.

Unfortunately compilation fails unless you have clang-14+ or gcc-13+,
but I sent a patch for that upstream. Other than that, it builds and all
tests pass locally on my x86-64 Linux machine.

The gn scraper regexes were broken preventing the compilation from
linking. Fix them. As a drive-by, also add additional conditionals for
compilation on 32-bit arm.

Refs: nodejs#50690
@joyeecheung
Copy link
Member

joyeecheung commented Nov 13, 2023

It seems the positive ones are mostly microbenchmarks that probably are not written in a way to eliminate compilation speed out of the measurement. So if it's Maglev kicking in instead of Turbofan, that might make a difference in a micro-benchmarking loop, but then in real world applications users don't usually run one API in a loop so that when or which compiler kicks in makes a significant difference. We should probably look into some more real-world CLIs for the measurement (e.g. #50684), where a mid-tier optimizing compiler could make a meaningful difference (A significant part of CLI startup is compilation. At the same time, Node.js core could also hit many hot paths trying to complete the loading process).

@targos
Copy link
Member

targos commented Nov 13, 2023

Gets us closer to Google Chrome which ships Maglev.

Can you show where it's enabled in Chrome's build config?

@kvakil
Copy link
Contributor Author

kvakil commented Nov 13, 2023

Gets us closer to Google Chrome which ships Maglev.

Can you show where it's enabled in Chrome's build config?

I don't know enough about Chrome's setup to answer that precisely. You can run Chrome with --js-flags=--help and see that --maglev is enabled by default. It likely comes from this default in v8's BUILD.gn:

node/deps/v8/BUILD.gn

Lines 505 to 508 in fc2862b

if (v8_enable_maglev == "") {
v8_enable_maglev = v8_enable_turbofan &&
(v8_current_cpu == "arm" || v8_current_cpu == "x64" ||
v8_current_cpu == "arm64")

@kvakil
Copy link
Contributor Author

kvakil commented Nov 13, 2023

It seems the positive ones are mostly microbenchmarks that probably are not written in a way to eliminate compilation speed out of the measurement. So if it's Maglev kicking in instead of Turbofan, that might make a difference in a micro-benchmarking loop, but then in real world applications users don't usually run one API in a loop so that when or which compiler kicks in makes a significant difference. We should probably look into some more real-world CLIs for the measurement (e.g. #50684), where a mid-tier optimizing compiler could make a meaningful difference (A significant part of CLI startup is compilation. At the same time, Node.js core could also hit many hot paths trying to complete the loading process).

I tried the startup benchmarks in that PR and there wasn't a significant difference with Maglev on/off unfortunately.

I guess what we need here is more information on "for each benchmark, which compilation tier does it end up in?" It is possible that some of them never reach Turbofan at all. We could a detailed analysis like this one, but it sounds like you think it would be better to try to find representative macro-benchmarks rather than continuing to look at the microbenchmarks.

@kvakil
Copy link
Contributor Author

kvakil commented Nov 14, 2023

On a small Typescript build job, it looks like --maglev is ~7-10% faster than --no-maglev:

--maglev
        2.77 real         5.75 user         0.43 sys
        2.79 real         5.75 user         0.44 sys
        2.78 real         5.80 user         0.47 sys
        2.80 real         5.74 user         0.46 sys
        2.79 real         5.69 user         0.44 sys
        2.81 real         5.76 user         0.45 sys
--no-maglev
        2.98 real         6.30 user         0.39 sys
        3.00 real         6.27 user         0.41 sys
        3.01 real         6.27 user         0.40 sys
        3.02 real         6.26 user         0.41 sys
        3.05 real         6.28 user         0.42 sys
        3.01 real         6.25 user         0.40 sys

I can't share the repo unfortunately, but will try to run typescript on some public repos to see if it generalizes.

@kvakil
Copy link
Contributor Author

kvakil commented Nov 14, 2023

Trying more Typescript projects. I chose xstate which is one the libraries that Typescript benchmarks for performance regressions against. I compiled node with v8-lkgr.

$ git clone https://github.com/statelyai/xstate.git
$ yarn install

and then I timed a typecheck after a clean:

$ node node_modules/.bin/tsc --build --clean && \time node node_modules/.bin/tsc --build

10 iterations of tsc --build with --maglev:

        3.90 real         7.33 user         0.32 sys                                                      
        3.87 real         7.29 user         0.30 sys                                                      
        3.87 real         7.30 user         0.30 sys                                                      
        3.91 real         7.34 user         0.31 sys                                                      
        3.90 real         7.30 user         0.29 sys                                                      
        3.87 real         7.28 user         0.31 sys                                                      
        3.89 real         7.31 user         0.30 sys                                                      
        3.92 real         7.35 user         0.31 sys                                                      
        3.86 real         7.27 user         0.31 sys                                                                                                                                                                
        4.03 real         7.49 user         0.31 sys                                                      

10 iterations of --no-maglev:

        4.79 real         8.52 user         0.29 sys 
        4.80 real         8.53 user         0.30 sys
        4.82 real         8.53 user         0.29 sys 
        4.87 real         8.63 user         0.29 sys                                                                                                                                                                
        4.79 real         8.53 user         0.29 sys                                                                                                                                                                
        4.82 real         8.55 user         0.29 sys                                                                                                                                                                
        4.92 real         8.66 user         0.31 sys                                                                                                                                                                
        4.81 real         8.52 user         0.30 sys                                                                                                                                                                
        4.81 real         8.53 user         0.30 sys                                                                                                                                                                
        4.79 real         8.52 user         0.29 sys                                                                                                                                                                

20% improvement in wall-clock and 14% improvement in CPU-time.

@targos
Copy link
Member

targos commented Nov 15, 2023

Gets us closer to Google Chrome which ships Maglev.

Can you show where it's enabled in Chrome's build config?

I don't know enough about Chrome's setup to answer that precisely. You can run Chrome with --js-flags=--help and see that --maglev is enabled by default. It likely comes from this default in v8's BUILD.gn:

node/deps/v8/BUILD.gn

Lines 505 to 508 in fc2862b

if (v8_enable_maglev == "") {
v8_enable_maglev = v8_enable_turbofan &&
(v8_current_cpu == "arm" || v8_current_cpu == "x64" ||
v8_current_cpu == "arm64")

OK. I'm fine with the proposal as long as we enable it by default on the same architectures as V8.

@kvakil
Copy link
Contributor Author

kvakil commented Nov 15, 2023

OK. I'm fine with the proposal as long as we enable it by default on the same architectures as V8.

Sure. Do you have an opinion on whether enabling it by default can happen in a minor release or if that should be a semver-major change?

kvakil added a commit that referenced this issue Nov 20, 2023
This adds a configuration flag to enable V8's Maglev compiler.

Unfortunately compilation fails unless you have clang-14+ or gcc-13+,
but I sent a patch for that upstream. Other than that, it builds and all
tests pass locally on my x86-64 Linux machine.

The gn scraper regexes were broken preventing the compilation from
linking. Fix them. As a drive-by, also add additional conditionals for
compilation on 32-bit arm.

Refs: #50690
PR-URL: #50692
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@kvakil
Copy link
Contributor Author

kvakil commented Nov 21, 2023

Another benchmark: eslint on specific directories of the Node.js codebase. Overall marginally (i.e. 2-4%) faster with maglev than without:

$ hyperfine -L maglev --maglev,--no-maglev './node_maglev {maglev} tools/node_modules/eslint/bin/eslint.js --max-warnings=0 --report-unused-disable-directives  lib'
Benchmark 1: ./node_maglev --maglev tools/node_modules/eslint/bin/eslint.js --max-warnings=0 --report-unused-disable-directives  lib
  Time (mean ± σ):     12.885 s ±  0.271 s    [User: 17.605 s, System: 0.775 s]
  Range (min … max):   12.578 s … 13.384 s    10 runs
 
Benchmark 2: ./node_maglev --no-maglev tools/node_modules/eslint/bin/eslint.js --max-warnings=0 --report-unused-disable-directives  lib
  Time (mean ± σ):     13.105 s ±  0.349 s    [User: 18.707 s, System: 0.688 s]
  Range (min … max):   12.762 s … 13.838 s    10 runs
 
Summary
  './node_maglev --maglev tools/node_modules/eslint/bin/eslint.js --max-warnings=0 --report-unused-disable-directives  lib' ran
    1.02 ± 0.03 times faster than './node_maglev --no-maglev tools/node_modules/eslint/bin/eslint.js --max-warnings=0 --report-unused-disable-directives  lib'
$ hyperfine -L maglev --maglev,--no-maglev './node_maglev {maglev} tools/node_modules/eslint/bin/eslint.js --max-warnings=0 --report-unused-disable-directives benchmark'
Benchmark 1: ./node_maglev --maglev tools/node_modules/eslint/bin/eslint.js --max-warnings=0 --report-unused-disable-directives benchmark
  Time (mean ± σ):      3.792 s ±  0.050 s    [User: 5.603 s, System: 0.351 s]
  Range (min … max):    3.716 s …  3.891 s    10 runs
 
Benchmark 2: ./node_maglev --no-maglev tools/node_modules/eslint/bin/eslint.js --max-warnings=0 --report-unused-disable-directives benchmark
  Time (mean ± σ):      3.943 s ±  0.061 s    [User: 5.899 s, System: 0.297 s]
  Range (min … max):    3.877 s …  4.055 s    10 runs
 
Summary
  './node_maglev --maglev tools/node_modules/eslint/bin/eslint.js --max-warnings=0 --report-unused-disable-directives benchmark' ran
    1.04 ± 0.02 times faster than './node_maglev --no-maglev tools/node_modules/eslint/bin/eslint.js --max-warnings=0 --report-unused-disable-directives benchmark'

targos pushed a commit that referenced this issue Nov 23, 2023
This adds a configuration flag to enable V8's Maglev compiler.

Unfortunately compilation fails unless you have clang-14+ or gcc-13+,
but I sent a patch for that upstream. Other than that, it builds and all
tests pass locally on my x86-64 Linux machine.

The gn scraper regexes were broken preventing the compilation from
linking. Fix them. As a drive-by, also add additional conditionals for
compilation on 32-bit arm.

Refs: #50690
PR-URL: #50692
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
martenrichter pushed a commit to martenrichter/node that referenced this issue Nov 26, 2023
This adds a configuration flag to enable V8's Maglev compiler.

Unfortunately compilation fails unless you have clang-14+ or gcc-13+,
but I sent a patch for that upstream. Other than that, it builds and all
tests pass locally on my x86-64 Linux machine.

The gn scraper regexes were broken preventing the compilation from
linking. Fix them. As a drive-by, also add additional conditionals for
compilation on 32-bit arm.

Refs: nodejs#50690
PR-URL: nodejs#50692
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
lucshi pushed a commit to lucshi/node that referenced this issue Nov 27, 2023
This adds a configuration flag to enable V8's Maglev compiler.

Unfortunately compilation fails unless you have clang-14+ or gcc-13+,
but I sent a patch for that upstream. Other than that, it builds and all
tests pass locally on my x86-64 Linux machine.

The gn scraper regexes were broken preventing the compilation from
linking. Fix them. As a drive-by, also add additional conditionals for
compilation on 32-bit arm.

Refs: nodejs#50690
PR-URL: nodejs#50692
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this issue Nov 29, 2023
This adds a configuration flag to enable V8's Maglev compiler.

Unfortunately compilation fails unless you have clang-14+ or gcc-13+,
but I sent a patch for that upstream. Other than that, it builds and all
tests pass locally on my x86-64 Linux machine.

The gn scraper regexes were broken preventing the compilation from
linking. Fix them. As a drive-by, also add additional conditionals for
compilation on 32-bit arm.

Refs: #50690
PR-URL: #50692
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this issue Nov 30, 2023
This adds a configuration flag to enable V8's Maglev compiler.

Unfortunately compilation fails unless you have clang-14+ or gcc-13+,
but I sent a patch for that upstream. Other than that, it builds and all
tests pass locally on my x86-64 Linux machine.

The gn scraper regexes were broken preventing the compilation from
linking. Fix them. As a drive-by, also add additional conditionals for
compilation on 32-bit arm.

Refs: #50690
PR-URL: #50692
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Talent30
Copy link

Talent30 commented Dec 8, 2023

As a mid-tier compiler Maglev may not bring much performance benefit for long-running server-like workloads.

The middle-tier compiler definitely has a positive impact on JS development tools(CLI apps).

UlisesGascon pushed a commit that referenced this issue Dec 11, 2023
This adds a configuration flag to enable V8's Maglev compiler.

Unfortunately compilation fails unless you have clang-14+ or gcc-13+,
but I sent a patch for that upstream. Other than that, it builds and all
tests pass locally on my x86-64 Linux machine.

The gn scraper regexes were broken preventing the compilation from
linking. Fix them. As a drive-by, also add additional conditionals for
compilation on 32-bit arm.

Refs: #50690
PR-URL: #50692
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
kvakil added a commit to kvakil/node that referenced this issue Dec 18, 2023
This allows Maglev (see ref) to build on the compilers we use.

Original commit message:

    [maglev] fix non-ptr-compr compilation on old compilers

    When pointer compression is disabled, the preprocessor expands some
    static asserts to static_assert(false), which doesn't compile on
    compilers not implementing the C++ defect report CWG2518, notably clang
    before version 17 and gcc before version 13.

    Adding in part of the template parameter to the static assert prevents
    it from being evaluated immediately which fixes the compilation.

    Test: compiled with gcc-11 and clang-14 without pointer compression.
    Change-Id: I95ce29bdb1278e6dad9e592d6f9476395f8aeb59
    Fixed: v8:14355
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5022760
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#91553}

Refs: v8/v8@de611e6
Refs: nodejs#50690
UlisesGascon pushed a commit that referenced this issue Dec 19, 2023
This adds a configuration flag to enable V8's Maglev compiler.

Unfortunately compilation fails unless you have clang-14+ or gcc-13+,
but I sent a patch for that upstream. Other than that, it builds and all
tests pass locally on my x86-64 Linux machine.

The gn scraper regexes were broken preventing the compilation from
linking. Fix them. As a drive-by, also add additional conditionals for
compilation on 32-bit arm.

Refs: #50690
PR-URL: #50692
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
kvakil added a commit to kvakil/node that referenced this issue Dec 19, 2023
This allows Maglev (see ref) to build on the compilers we use.

Original commit message:

    [maglev] fix non-ptr-compr compilation on old compilers

    When pointer compression is disabled, the preprocessor expands some
    static asserts to static_assert(false), which doesn't compile on
    compilers not implementing the C++ defect report CWG2518, notably clang
    before version 17 and gcc before version 13.

    Adding in part of the template parameter to the static assert prevents
    it from being evaluated immediately which fixes the compilation.

    Test: compiled with gcc-11 and clang-14 without pointer compression.
    Change-Id: I95ce29bdb1278e6dad9e592d6f9476395f8aeb59
    Fixed: v8:14355
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5022760
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#91553}

Refs: v8/v8@de611e6
Refs: nodejs#50690
nodejs-github-bot pushed a commit that referenced this issue Dec 21, 2023
Original commit message:

    [maglev] fix non-ptr-compr compilation on old compilers

    When pointer compression is disabled, the preprocessor expands some
    static asserts to static_assert(false), which doesn't compile on
    compilers not implementing the C++ defect report CWG2518, notably clang
    before version 17 and gcc before version 13.

    Adding in part of the template parameter to the static assert prevents
    it from being evaluated immediately which fixes the compilation.

    Test: compiled with gcc-11 and clang-14 without pointer compression.
    Change-Id: I95ce29bdb1278e6dad9e592d6f9476395f8aeb59
    Fixed: v8:14355
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5022760
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#91553}

Refs: v8/v8@de611e6
PR-URL: #51200
Refs: #50690
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
targos pushed a commit to targos/node that referenced this issue Dec 22, 2023
Original commit message:

    [maglev] fix non-ptr-compr compilation on old compilers

    When pointer compression is disabled, the preprocessor expands some
    static asserts to static_assert(false), which doesn't compile on
    compilers not implementing the C++ defect report CWG2518, notably clang
    before version 17 and gcc before version 13.

    Adding in part of the template parameter to the static assert prevents
    it from being evaluated immediately which fixes the compilation.

    Test: compiled with gcc-11 and clang-14 without pointer compression.
    Change-Id: I95ce29bdb1278e6dad9e592d6f9476395f8aeb59
    Fixed: v8:14355
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5022760
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#91553}

Refs: v8/v8@de611e6
PR-URL: nodejs#51200
Refs: nodejs#50690
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
targos pushed a commit to targos/node that referenced this issue Dec 22, 2023
Original commit message:

    [maglev] fix non-ptr-compr compilation on old compilers

    When pointer compression is disabled, the preprocessor expands some
    static asserts to static_assert(false), which doesn't compile on
    compilers not implementing the C++ defect report CWG2518, notably clang
    before version 17 and gcc before version 13.

    Adding in part of the template parameter to the static assert prevents
    it from being evaluated immediately which fixes the compilation.

    Test: compiled with gcc-11 and clang-14 without pointer compression.
    Change-Id: I95ce29bdb1278e6dad9e592d6f9476395f8aeb59
    Fixed: v8:14355
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5022760
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#91553}

Refs: v8/v8@de611e6
PR-URL: nodejs#51200
Refs: nodejs#50690
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
targos pushed a commit to targos/node that referenced this issue Dec 22, 2023
Original commit message:

    [maglev] fix non-ptr-compr compilation on old compilers

    When pointer compression is disabled, the preprocessor expands some
    static asserts to static_assert(false), which doesn't compile on
    compilers not implementing the C++ defect report CWG2518, notably clang
    before version 17 and gcc before version 13.

    Adding in part of the template parameter to the static assert prevents
    it from being evaluated immediately which fixes the compilation.

    Test: compiled with gcc-11 and clang-14 without pointer compression.
    Change-Id: I95ce29bdb1278e6dad9e592d6f9476395f8aeb59
    Fixed: v8:14355
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5022760
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#91553}

Refs: v8/v8@de611e6
PR-URL: nodejs#51200
Refs: nodejs#50690
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
targos pushed a commit to targos/node that referenced this issue Dec 22, 2023
Original commit message:

    [maglev] fix non-ptr-compr compilation on old compilers

    When pointer compression is disabled, the preprocessor expands some
    static asserts to static_assert(false), which doesn't compile on
    compilers not implementing the C++ defect report CWG2518, notably clang
    before version 17 and gcc before version 13.

    Adding in part of the template parameter to the static assert prevents
    it from being evaluated immediately which fixes the compilation.

    Test: compiled with gcc-11 and clang-14 without pointer compression.
    Change-Id: I95ce29bdb1278e6dad9e592d6f9476395f8aeb59
    Fixed: v8:14355
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5022760
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#91553}

Refs: v8/v8@de611e6
PR-URL: nodejs#51200
Refs: nodejs#50690
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
RafaelGSS pushed a commit that referenced this issue Jan 2, 2024
Original commit message:

    [maglev] fix non-ptr-compr compilation on old compilers

    When pointer compression is disabled, the preprocessor expands some
    static asserts to static_assert(false), which doesn't compile on
    compilers not implementing the C++ defect report CWG2518, notably clang
    before version 17 and gcc before version 13.

    Adding in part of the template parameter to the static assert prevents
    it from being evaluated immediately which fixes the compilation.

    Test: compiled with gcc-11 and clang-14 without pointer compression.
    Change-Id: I95ce29bdb1278e6dad9e592d6f9476395f8aeb59
    Fixed: v8:14355
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5022760
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#91553}

Refs: v8/v8@de611e6
PR-URL: #51200
Refs: #50690
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
kvakil added a commit to kvakil/node that referenced this issue 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: nodejs#50690
targos pushed a commit that referenced this issue Jan 4, 2024
Original commit message:

    [maglev] fix non-ptr-compr compilation on old compilers

    When pointer compression is disabled, the preprocessor expands some
    static asserts to static_assert(false), which doesn't compile on
    compilers not implementing the C++ defect report CWG2518, notably clang
    before version 17 and gcc before version 13.

    Adding in part of the template parameter to the static assert prevents
    it from being evaluated immediately which fixes the compilation.

    Test: compiled with gcc-11 and clang-14 without pointer compression.
    Change-Id: I95ce29bdb1278e6dad9e592d6f9476395f8aeb59
    Fixed: v8:14355
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5022760
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#91553}

Refs: v8/v8@de611e6
PR-URL: #51200
Refs: #50690
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
PR-URL: #50115
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
nodejs-github-bot pushed a commit that referenced this issue Jan 8, 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
PR-URL: #51360
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this issue Jan 12, 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: nodejs#50690
PR-URL: nodejs#51360
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Medhansh404 pushed a commit to Medhansh404/node that referenced this issue Jan 19, 2024
Original commit message:

    [maglev] fix non-ptr-compr compilation on old compilers

    When pointer compression is disabled, the preprocessor expands some
    static asserts to static_assert(false), which doesn't compile on
    compilers not implementing the C++ defect report CWG2518, notably clang
    before version 17 and gcc before version 13.

    Adding in part of the template parameter to the static assert prevents
    it from being evaluated immediately which fixes the compilation.

    Test: compiled with gcc-11 and clang-14 without pointer compression.
    Change-Id: I95ce29bdb1278e6dad9e592d6f9476395f8aeb59
    Fixed: v8:14355
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5022760
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#91553}

Refs: v8/v8@de611e6
PR-URL: nodejs#51200
Refs: nodejs#50690
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
PR-URL: nodejs#50115
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Medhansh404 pushed a commit to Medhansh404/node that referenced this issue Jan 19, 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: nodejs#50690
PR-URL: nodejs#51360
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Original commit message:

    [maglev] fix non-ptr-compr compilation on old compilers

    When pointer compression is disabled, the preprocessor expands some
    static asserts to static_assert(false), which doesn't compile on
    compilers not implementing the C++ defect report CWG2518, notably clang
    before version 17 and gcc before version 13.

    Adding in part of the template parameter to the static assert prevents
    it from being evaluated immediately which fixes the compilation.

    Test: compiled with gcc-11 and clang-14 without pointer compression.
    Change-Id: I95ce29bdb1278e6dad9e592d6f9476395f8aeb59
    Fixed: v8:14355
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5022760
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#91553}

Refs: v8/v8@de611e6
PR-URL: #51200
Refs: #50690
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@RedYetiDev RedYetiDev moved this from Awaiting Triage to Done in Node.js feature requests Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants