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

deps: V8: cherry-pick 687d865fe251 #31007

Closed

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Dec 17, 2019

Opening this to test performance and memory benefits. Upd: good so far, marking as ready.

This should reduce the memory load and improve the speed in cases that heavily rely on Buffer usage.

Refs: v8/v8@687d865
Refs: #1671

Original commit message:

[heap] Perform GCs on v8::BackingStore allocation

This adds heuristics to perform young and full GCs on allocation
of external ArrayBuffer backing stores.

Young GCs are performed proactively based on the external backing
store bytes for the young generation. Full GCs are performed only
if the allocation fails. Subsequent CLs will add heuristics to
start incremental full GCs based on the external backing store bytes.

This will allow us to remove AdjustAmountOfExternalMemory for
ArrayBuffers.

Bug: v8:9701, chromium:1008938
Change-Id: I0e8688f582989518926c38260b5cf14e2ca93f84
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1803614
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65480}

@ChALkeR ChALkeR added v8 engine Issues and PRs related to the V8 dependency. memory Issues and PRs related to the memory management or memory footprint. performance Issues and PRs related to the performance of Node.js. labels Dec 17, 2019
@ChALkeR ChALkeR requested review from mscdex and targos December 17, 2019 18:52
@ChALkeR
Copy link
Member Author

ChALkeR commented Dec 17, 2019

/cc @nodejs/benchmarking

@ChALkeR
Copy link
Member Author

ChALkeR commented Dec 17, 2019

Benchmarks (updated, complete):

buffers (complete):

compare-instance-method n=1000000 args=5 size=16                                *   7.00 %   ±6.81%  ±9.14% ±12.08%
compare-offset n=1000000 size=16386 method='slice'                              *  -6.24 %   ±5.99%  ±8.05% ±10.63%
concat n=800000 withTotalLength=0 pieceSize=256 pieces=16                     ***  24.22 %  ±11.06% ±14.71% ±19.15%
concat n=800000 withTotalLength=1 pieceSize=1 pieces=4                          *   1.96 %   ±1.65%  ±2.19%  ±2.86%
concat n=800000 withTotalLength=1 pieceSize=256 pieces=16                      **  19.40 %  ±11.76% ±15.65% ±20.37%
copy n=6000000 partial='false' bytes=8                                          *  -2.07 %   ±1.78%  ±2.37%  ±3.08%
copy n=6000000 partial='true' bytes=128                                         *   2.65 %   ±2.34%  ±3.12%  ±4.07%
creation n=600000 len=4096 type='slow-allocUnsafe'                              *  11.27 %   ±8.64% ±11.51% ±15.00%
creation n=600000 len=8192 type='fast-alloc'                                  ***  23.01 %   ±8.84% ±11.85% ±15.62%
creation n=600000 len=8192 type='fast-alloc-fill'                             ***  21.18 %   ±7.48% ±10.02% ±13.19%
creation n=600000 len=8192 type='fast-allocUnsafe'                            ***  10.57 %   ±4.46%  ±5.95%  ±7.77%
creation n=600000 len=8192 type='slow-allocUnsafe'                            ***  14.68 %   ±5.57%  ±7.44%  ±9.75%
fill n=20000 size=8192 type='fill(0)'                                           *   6.23 %   ±5.83%  ±7.76% ±10.11%
fill n=20000 size=8192 type='fill(Buffer.alloc(1), 0)'                          *   7.37 %   ±6.00%  ±8.02% ±10.49%
fill n=20000 size=8192 type='fill("t", "utf8")'                                 *  -6.35 %   ±5.94%  ±7.91% ±10.29%
indexof n=50000 type='string' encoding='ucs2' search='Alice'                    *  -7.07 %   ±7.02%  ±9.35% ±12.19%
indexof n=50000 type='string' encoding='ucs2' search='</i> to the Caterpillar'  *  -2.96 %   ±2.35%  ±3.12%  ±4.06%
indexof n=50000 type='string' encoding='ucs2' search='Ou est ma chatte?'        *  -8.47 %   ±6.75%  ±9.00% ±11.76%
normalize-encoding n=1000000 encoding='UTF16LE'                                 *   6.06 %   ±5.45%  ±7.27%  ±9.48%
read-with-byteLength byteLength=1 n=1000000 type='IntLE' buffer='fast'          *  -9.24 %   ±9.09% ±12.19% ±16.04%
swap n=1000000 len=2056 method='swap32' aligned='false'                       ***  10.16 %   ±4.74%  ±6.32%  ±8.24%
swap n=1000000 len=768 method='swap16' aligned='false'                          *  -2.86 %   ±2.86%  ±3.80%  ±4.95%
dataview-set n=1000000 type='Uint16BE'                                          *  -6.84 %   ±5.55%  ±7.41%  ±9.71%

18.85 false positives, when considering a  5% risk acceptance (*, **, ***),
3.77 false positives, when considering a   1% risk acceptance (**, ***),
0.38 false positives, when considering a 0.1% risk acceptance (***)

Most of * are likely flukes (there was a large number of benchmarks in the whole buffer set), see the expected false positives count.

net (complete):

net-c2s-cork dur=5 type='buf' len=1024                                                *  -16.61 %  ±14.59% ±19.41% ±25.27%
net-c2s dur=5 type='utf' len=64                                                       *    4.34 %   ±3.86%  ±5.14%  ±6.70%
net-pipe dur=5 type='asc' len=64                                                    ***   10.26 %   ±5.37%  ±7.16%  ±9.35%
net-s2c dur=5 recvbufgenfn='false' recvbuflen=0 type='buf' sendchunklen=256           *    5.83 %   ±5.10%  ±6.81%  ±8.90%
net-s2c dur=5 recvbufgenfn='false' recvbuflen=1048576 type='buf' sendchunklen=256     *   -2.24 %   ±2.11%  ±2.80%  ±3.65%
net-s2c dur=5 recvbufgenfn='false' recvbuflen=1048576 type='utf' sendchunklen=131072  *    1.97 %   ±1.81%  ±2.41%  ±3.14%
net-s2c dur=5 recvbufgenfn='true' recvbuflen=65536 type='buf' sendchunklen=131072     *    2.23 %   ±1.70%  ±2.26%  ±2.94%
tcp-raw-s2c dur=5 type='asc' len=16777216                                             *   -4.18 %   ±4.18%  ±5.56%  ±7.24%
tcp-raw-s2c dur=5 type='utf' len=16777216                                             *   -3.49 %   ±3.32%  ±4.42%  ±5.76%

6.25 false positives, when considering a   5% risk acceptance (*, **, ***),
1.25 false positives, when considering a   1% risk acceptance (**, ***),
0.12 false positives, when considering a 0.1% risk acceptance (***)

Again, most of * are likely flukes.

streams/pipe

Nothing.

fs/stream-throughput

read-stream-throughput size=1024 filesize=1048576000 encodingType='asc'      *   6.03 %       ±5.46%  ±7.27%   ±9.47%
read-stream-throughput size=1024 filesize=1048576000 encodingType='utf'     **   6.60 %       ±4.70%  ±6.28%   ±8.22%
read-stream-throughput size=1048576 filesize=1048576000 encodingType='asc'   *  10.39 %       ±7.98% ±10.63%  ±13.87%
read-stream-throughput size=1048576 filesize=1048576000 encodingType='buf'   *  12.97 %      ±11.28% ±15.02%  ±19.56%
read-stream-throughput size=65535 filesize=1048576000 encodingType='buf'    **  13.90 %      ±10.00% ±13.31%  ±17.33%

1.20 false positives, when considering a   5% risk acceptance (*, **, ***),
0.24 false positives, when considering a   1% risk acceptance (**, ***),
0.02 false positives, when considering a 0.1% risk acceptance (***)

@ChALkeR ChALkeR added the buffer Issues and PRs related to the buffer subsystem. label Dec 17, 2019
@ChALkeR ChALkeR marked this pull request as ready for review December 18, 2019 10:20
@ChALkeR ChALkeR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v10.x labels Dec 19, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 20, 2019

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 25, 2019

BridgeAR pushed a commit that referenced this pull request Dec 25, 2019
Original commit message:

    [heap] Perform GCs on v8::BackingStore allocation

    This adds heuristics to perform young and full GCs on allocation
    of external ArrayBuffer backing stores.

    Young GCs are performed proactively based on the external backing
    store bytes for the young generation. Full GCs are performed only
    if the allocation fails. Subsequent CLs will add heuristics to
    start incremental full GCs based on the external backing store bytes.

    This will allow us to remove AdjustAmountOfExternalMemory for
    ArrayBuffers.

    Bug: v8:9701, chromium:1008938
    Change-Id: I0e8688f582989518926c38260b5cf14e2ca93f84
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1803614
    Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
    Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
    Reviewed-by: Hannes Payer <hpayer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#65480}

PR-URL: #31007
Refs: v8/v8@687d865
Refs: #1671
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR

This comment has been minimized.

@BridgeAR BridgeAR closed this Dec 25, 2019
@BridgeAR
Copy link
Member

I forced pushed it out again after realizing that the embedder string has not been updated.

@BridgeAR BridgeAR reopened this Dec 25, 2019
@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 25, 2019
@ChALkeR
Copy link
Member Author

ChALkeR commented Dec 26, 2019

@BridgeAR Oh, my bad re: v8_embedder_string.

The problem is that it did not work -- it's present in master: https://github.com/nodejs/node/commits/master

Will file two alternative PRs to fix that now.

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Dec 26, 2019
0e21c1e has landed without a proper
v8_embedder_string bump, this is a follow-up fix.

Refs: 0e21c1e
Refs: nodejs#31007
Original commit message:

    [heap] Perform GCs on v8::BackingStore allocation

    This adds heuristics to perform young and full GCs on allocation
    of external ArrayBuffer backing stores.

    Young GCs are performed proactively based on the external backing
    store bytes for the young generation. Full GCs are performed only
    if the allocation fails. Subsequent CLs will add heuristics to
    start incremental full GCs based on the external backing store bytes.

    This will allow us to remove AdjustAmountOfExternalMemory for
    ArrayBuffers.

    Bug: v8:9701, chromium:1008938
    Change-Id: I0e8688f582989518926c38260b5cf14e2ca93f84
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1803614
    Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
    Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
    Reviewed-by: Hannes Payer <hpayer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#65480}

Refs: v8/v8@687d865
Refs: nodejs#1671
@ChALkeR ChALkeR force-pushed the chalker/v8-cherrypick-scavenges branch from f707f1e to b7fa732 Compare December 26, 2019 02:35
@ChALkeR
Copy link
Member Author

ChALkeR commented Dec 26, 2019

This PR is now updated to include v8_embedder_string change, but it can land only after a revert (#31098).

An alternative is to update v8_embedder_string separately, PR: #31096.

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Dec 26, 2019
This reverts commit 0e21c1e.

It landed without a proper v8_embedder_string bump, reverting
to re-land cleanly.

Refs: 0e21c1e
Refs: nodejs#31007
@BridgeAR
Copy link
Member

@ChALkeR thanks for following up on this! I guess I force pushed to my fork instead of upstream.

BridgeAR pushed a commit that referenced this pull request Jan 1, 2020
0e21c1e has landed without a proper
v8_embedder_string bump, this is a follow-up fix.

PR-URL: #31096
Refs: 0e21c1e
Refs: #31007
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020

Closing as resolved.

@BridgeAR BridgeAR closed this Jan 2, 2020
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
Original commit message:

    [heap] Perform GCs on v8::BackingStore allocation

    This adds heuristics to perform young and full GCs on allocation
    of external ArrayBuffer backing stores.

    Young GCs are performed proactively based on the external backing
    store bytes for the young generation. Full GCs are performed only
    if the allocation fails. Subsequent CLs will add heuristics to
    start incremental full GCs based on the external backing store bytes.

    This will allow us to remove AdjustAmountOfExternalMemory for
    ArrayBuffers.

    Bug: v8:9701, chromium:1008938
    Change-Id: I0e8688f582989518926c38260b5cf14e2ca93f84
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1803614
    Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
    Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
    Reviewed-by: Hannes Payer <hpayer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#65480}

PR-URL: #31007
Refs: v8/v8@687d865
Refs: #1671
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
0e21c1e has landed without a proper
v8_embedder_string bump, this is a follow-up fix.

PR-URL: #31096
Refs: 0e21c1e
Refs: #31007
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
@targos
Copy link
Member

targos commented Jan 14, 2020

This needs a backport to land on v12.x-staging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. memory Issues and PRs related to the memory management or memory footprint. 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 this pull request may close these issues.

7 participants