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

url: using util._extend for improving profermace #16081

Closed
wants to merge 1 commit into
base: master
from

Conversation

@starkwang
Contributor

starkwang commented Oct 8, 2017

Object.assign is much slower than util._extend according to
the refs. This change is to convert the Object.assign to use
util._extend in url module for improving profermance.

Refs: nodejs/CTC#62

Benchmark:

 url/url-resolve.js n=100000 path="down" href="auth"                                                     0.73 %            4.001573e-01
 url/url-resolve.js n=100000 path="down" href="dot"                                                     -0.28 %            8.223166e-01
 url/url-resolve.js n=100000 path="down" href="file"                                                     1.69 %            1.027301e-01
 url/url-resolve.js n=100000 path="down" href="idn"                                                      0.87 %            4.341655e-01
 url/url-resolve.js n=100000 path="down" href="javascript"                                               3.48 %          * 4.274686e-02
 url/url-resolve.js n=100000 path="down" href="long"                                                     1.36 %            1.400734e-01
 url/url-resolve.js n=100000 path="down" href="noscheme"                                                -2.57 %            7.697437e-02
 url/url-resolve.js n=100000 path="down" href="percent"                                                  0.73 %            5.244919e-01
 url/url-resolve.js n=100000 path="down" href="short"                                                    4.44 %        *** 3.820092e-04
 url/url-resolve.js n=100000 path="down" href="ws"                                                       1.59 %          * 2.243749e-02
 url/url-resolve.js n=100000 path="foo/bar" href="auth"                                                  1.94 %          * 4.641876e-02
 url/url-resolve.js n=100000 path="foo/bar" href="dot"                                                   5.24 %        *** 5.656288e-05
 url/url-resolve.js n=100000 path="foo/bar" href="file"                                                  2.64 %            9.331631e-02
 url/url-resolve.js n=100000 path="foo/bar" href="idn"                                                  -0.45 %            6.854409e-01
 url/url-resolve.js n=100000 path="foo/bar" href="javascript"                                           -1.07 %            5.415987e-01
 url/url-resolve.js n=100000 path="foo/bar" href="long"                                                  0.49 %            5.780417e-01
 url/url-resolve.js n=100000 path="foo/bar" href="noscheme"                                              5.08 %         ** 5.798221e-03
 url/url-resolve.js n=100000 path="foo/bar" href="percent"                                               0.04 %            9.816567e-01
 url/url-resolve.js n=100000 path="foo/bar" href="short"                                                 2.21 %            1.986360e-01
 url/url-resolve.js n=100000 path="foo/bar" href="ws"                                                    0.29 %            7.859900e-01
 url/url-resolve.js n=100000 path="sibling" href="auth"                                                  1.95 %          * 3.832157e-02
 url/url-resolve.js n=100000 path="sibling" href="dot"                                                   1.83 %            7.399938e-02
 url/url-resolve.js n=100000 path="sibling" href="file"                                                  2.44 %          * 3.722787e-02
 url/url-resolve.js n=100000 path="sibling" href="idn"                                                   0.83 %            3.692427e-01
 url/url-resolve.js n=100000 path="sibling" href="javascript"                                            0.09 %            9.523714e-01
 url/url-resolve.js n=100000 path="sibling" href="long"                                                  1.80 %            6.542290e-02
 url/url-resolve.js n=100000 path="sibling" href="noscheme"                                             -0.20 %            8.973654e-01
 url/url-resolve.js n=100000 path="sibling" href="percent"                                               0.57 %            6.611122e-01
 url/url-resolve.js n=100000 path="sibling" href="short"                                                 0.94 %            4.288303e-01
 url/url-resolve.js n=100000 path="sibling" href="ws"                                                    1.74 %          * 3.850364e-02
 url/url-resolve.js n=100000 path="up" href="auth"                                                       1.56 %            1.695226e-01
 url/url-resolve.js n=100000 path="up" href="dot"                                                        1.59 %            5.566368e-02
 url/url-resolve.js n=100000 path="up" href="file"                                                       0.03 %            9.810652e-01
 url/url-resolve.js n=100000 path="up" href="idn"                                                        0.58 %            4.708234e-01
 url/url-resolve.js n=100000 path="up" href="javascript"                                                 2.68 %          * 1.764476e-02
 url/url-resolve.js n=100000 path="up" href="long"                                                      -0.23 %            8.000201e-01
 url/url-resolve.js n=100000 path="up" href="noscheme"                                                   0.57 %            7.034992e-01
 url/url-resolve.js n=100000 path="up" href="percent"                                                    1.69 %            1.894380e-01
 url/url-resolve.js n=100000 path="up" href="short"                                                     -0.94 %            4.023246e-01
 url/url-resolve.js n=100000 path="up" href="ws"                                                         1.01 %            2.854117e-01
 url/url-resolve.js n=100000 path="withscheme" href="auth"                                               0.52 %            7.065682e-01
 url/url-resolve.js n=100000 path="withscheme" href="dot"                                                0.44 %            7.631471e-01
 url/url-resolve.js n=100000 path="withscheme" href="file"                                              -0.22 %            8.804126e-01
 url/url-resolve.js n=100000 path="withscheme" href="idn"                                                3.35 %          * 1.185270e-02
 url/url-resolve.js n=100000 path="withscheme" href="javascript"                                         0.01 %            9.954955e-01
 url/url-resolve.js n=100000 path="withscheme" href="long"                                               0.80 %            3.409249e-01
 url/url-resolve.js n=100000 path="withscheme" href="noscheme"                                          -0.60 %            7.459196e-01
 url/url-resolve.js n=100000 path="withscheme" href="percent"                                            0.83 %            6.370433e-01
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

url

url: using util._extend for improving profermace
`Object.assign` is much slower than `util._extend` according to
the refs. This change is to convert the `Object.assign` to use
`util._extend` in url module for improving profermance.

Refs: nodejs/CTC#62
@addaleax

/cc @mscdex

@hiroppy

hiroppy approved these changes Oct 8, 2017

@cjihrig

cjihrig approved these changes Oct 8, 2017

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Oct 8, 2017

Contributor

LGTM

Contributor

mscdex commented Oct 8, 2017

LGTM

@hiroppy

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell approved these changes Oct 9, 2017

LGTM.
@bmeurer ... this might be another case worth exploring to see why Object.assign() is slower. I'd much prefer to be using assign here.

@bmeurer

This comment has been minimized.

Show comment
Hide comment
@bmeurer

bmeurer Oct 10, 2017

Member

@jasnell The fact that Object.assign() is so much slower than handwritten for..in / Object.keys based loops is bugging me as well. We already optimized it somewhat, but I guess we could invest some more time into this (tracking bug).

Member

bmeurer commented Oct 10, 2017

@jasnell The fact that Object.assign() is so much slower than handwritten for..in / Object.keys based loops is bugging me as well. We already optimized it somewhat, but I guess we could invest some more time into this (tracking bug).

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Oct 10, 2017

Member

@bmeurer ...I owe you the beverage of your choice for all the work you've done optimizing things.

Member

jasnell commented Oct 10, 2017

@bmeurer ...I owe you the beverage of your choice for all the work you've done optimizing things.

@starkwang

This comment has been minimized.

Show comment
Hide comment
@starkwang

starkwang Oct 10, 2017

Contributor

There are still some code using Object.assign in lib, such as:
https://github.com/nodejs/node/blob/master/lib/child_process.js#L124
https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L836

Should we convert them into using util._extend temporarily, until the V8 team fix the profermace of assign?

Contributor

starkwang commented Oct 10, 2017

There are still some code using Object.assign in lib, such as:
https://github.com/nodejs/node/blob/master/lib/child_process.js#L124
https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L836

Should we convert them into using util._extend temporarily, until the V8 team fix the profermace of assign?

@bmeurer

This comment has been minimized.

Show comment
Hide comment
@bmeurer

bmeurer Oct 10, 2017

Member

@jasnell I'd say coffee or beer, but at this point I'm not allowed to drink either.

@starkwang Don't hold your breath for this. It'll probably take at least until V8 6.4, maybe even longer to get this on par (@camillobruni is on it). So maybe for Node 8 just go with util._extend.

Member

bmeurer commented Oct 10, 2017

@jasnell I'd say coffee or beer, but at this point I'm not allowed to drink either.

@starkwang Don't hold your breath for this. It'll probably take at least until V8 6.4, maybe even longer to get this on par (@camillobruni is on it). So maybe for Node 8 just go with util._extend.

@refack refack added the performance label Oct 10, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Oct 10, 2017

Member

Should we convert them into using util._extend temporarily, until the V8 team fix the profermace of assign?

IMHO it's not worthwhile in I/O wrappers (like child_process or http2) but only in places like this or path or other modules that are sync and JS heavy.

Member

refack commented Oct 10, 2017

Should we convert them into using util._extend temporarily, until the V8 team fix the profermace of assign?

IMHO it's not worthwhile in I/O wrappers (like child_process or http2) but only in places like this or path or other modules that are sync and JS heavy.

@refack

refack approved these changes Oct 10, 2017

@refack refack self-assigned this Oct 10, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Oct 10, 2017

Member

Landed in c1cd731

Member

refack commented Oct 10, 2017

Landed in c1cd731

@refack refack closed this Oct 10, 2017

refack added a commit that referenced this pull request Oct 10, 2017

url: using util._extend for improving profermace
`Object.assign` is much slower than `util._extend` according to
the refs. This change is to convert the `Object.assign` to use
`util._extend` in url module for improving profermance.

PR-URL: #16081
Refs: nodejs/CTC#62
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Oct 10, 2017

Contributor

FWIW I think we could make many of these util._extend()/Object.assign() use cases much faster if/when we can safely use obj->Clone(). @bmeurer Any thoughts on the suggestions in the related V8 issue here?

Contributor

mscdex commented Oct 10, 2017

FWIW I think we could make many of these util._extend()/Object.assign() use cases much faster if/when we can safely use obj->Clone(). @bmeurer Any thoughts on the suggestions in the related V8 issue here?

@bmeurer

This comment has been minimized.

Show comment
Hide comment
@bmeurer

bmeurer Oct 11, 2017

Member

@mscdex I don't really like that idea, especially since I'd love to get rid of Clone().

I was thinking that {...obj} might be a better alternative here. We could have a special bytecode for this and turn it into highly optimized code in the monomorphic case, but also handle cloning better in general, since the spread syntax uses CreateDataProperty instead of [[Set]].

Member

bmeurer commented Oct 11, 2017

@mscdex I don't really like that idea, especially since I'd love to get rid of Clone().

I was thinking that {...obj} might be a better alternative here. We could have a special bytecode for this and turn it into highly optimized code in the monomorphic case, but also handle cloning better in general, since the spread syntax uses CreateDataProperty instead of [[Set]].

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Oct 11, 2017

Member

Oh, I hadn't even considered the {...Obj} option.... I like it.

Member

jasnell commented Oct 11, 2017

Oh, I hadn't even considered the {...Obj} option.... I like it.

addaleax added a commit to addaleax/ayo that referenced this pull request Oct 12, 2017

url: using util._extend for improving profermace
`Object.assign` is much slower than `util._extend` according to
the refs. This change is to convert the `Object.assign` to use
`util._extend` in url module for improving profermance.

PR-URL: nodejs/node#16081
Refs: nodejs/CTC#62
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>

targos added a commit that referenced this pull request Oct 18, 2017

url: using util._extend for improving profermace
`Object.assign` is much slower than `util._extend` according to
the refs. This change is to convert the `Object.assign` to use
`util._extend` in url module for improving profermance.

PR-URL: #16081
Refs: nodejs/CTC#62
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>

@BridgeAR BridgeAR referenced this pull request Feb 14, 2018

Closed

http2: use `util._extend` instead of `Object.assign` #18766

3 of 3 tasks complete

@refack refack removed their assignment Oct 12, 2018

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