lib: make FreeList faster by apapirovski · Pull Request #27021 · nodejs/node

apapirovski · 2019-03-30T22:29:44Z

1st commit makes FreeList alloc faster by using Reflect.apply and removing is_reused_symbol.

2nd commit fixes a benchmark that I needed to run to confirm the results.

CI: https://ci.nodejs.org/job/node-test-pull-request/22260/
Benchmarks: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/312/

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2019-03-30T22:29:45Z

@apapirovski build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/3115/pipeline

apapirovski · 2019-03-31T00:14:22Z

Benchmark results

18:50:02 confidence improvement accuracy (*) (**) (***)
18:50:02 misc/freelist.js n=100000 *** 13.46 % ±1.82% ±2.40% ±3.08%

targos · 2019-03-31T17:14:40Z

Suggestion for the commit title: "lib: make FreeList faster"

nodejs-github-bot · 2019-03-31T20:46:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22079/

nodejs-github-bot · 2019-03-31T22:12:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22082/

Trott · 2019-03-31T23:59:13Z

Because these tests are often forgotten until the nightly run: Please make sure test/benchmark/test-benchmark-http.js and test/benchmark/test-benchmark-misc.js still pass with these changes.

BridgeAR · 2019-04-01T20:12:14Z

Please make sure test/benchmark/test-benchmark-http.js and test/benchmark/test-benchmark-misc.js still pass with these changes.

The test will continue to pass.

danbev · 2019-04-04T05:39:38Z

I'm getting the following lint error locally for this:

$ make lint-jsRunning JS linter.../Users/danielbevenius/work/nodejs/node/lib/internal/freelist.js 18:7 error Unexpected use of 'Reflect' no-restricted-globals✖ 1 problem (1 error, 0 warnings)

Not sure why this is not picked up by CI. I'm happy to add an ignore to this line but wanted to bring it up here first.

BridgeAR · 2019-04-04T05:47:02Z

@danbev the reason is that the rule just landed recently and the CI ran before the rule landed. Instead of adding an exception it would be ideal to use the primordials (const { Reflect } = primordials;).

targos · 2019-04-04T06:16:39Z

I opened #27083 to make the error message clearer.

Make FreeList faster by using Reflect.apply and not using is_reused_symbol, but rather just checking whether any items are present in the list prior to calling alloc.

nodejs-github-bot · 2019-04-06T22:49:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22260/

nodejs-github-bot · 2019-04-10T21:31:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22355/

danbev · 2019-04-11T04:00:34Z

Landed in 47f5cc1, and 29d0b43.

Make FreeList faster by using Reflect.apply and not using is_reused_symbol, but rather just checking whether any items are present in the list prior to calling alloc. PR-URL: #27021 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #27021 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Mar 30, 2019

apapirovski force-pushed the faster-freelist branch 2 times, most recently from 1ab7867 to 18428c9Compare March 30, 2019 22:37

addaleax approved these changes Mar 30, 2019
View reviewed changes

Comment threadlib/_http_client.js Outdated

cjihrig approved these changes Mar 31, 2019
View reviewed changes

devsnek approved these changes Mar 31, 2019
View reviewed changes

BridgeAR approved these changes Mar 31, 2019
View reviewed changes

Comment threadbenchmark/http/headers.js Outdated

lpinca approved these changes Mar 31, 2019
View reviewed changes

lpinca reviewed Mar 31, 2019
View reviewed changes

Comment threadbenchmark/misc/freelist.js Outdated

apapirovski changed the title ~~lib: faster FreeList~~lib: make FreeList fasterMar 31, 2019

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 1, 2019

apapirovski added 2 commits April 6, 2019 10:31

lib: faster FreeList
a8e8d52
Make FreeList faster by using Reflect.apply and not using is_reused_symbol, but rather just checking whether any items are present in the list prior to calling alloc.

benchmark: fix http headers benchmark
4409bf2

apapirovski force-pushed the faster-freelist branch from 3491ab3 to 4409bf2Compare April 6, 2019 17:31

BeniCheni mentioned this pull request Apr 8, 2019
benchmark: add "byGroup" config option #26559
Closed
4 tasks

jasnell approved these changes Apr 8, 2019
View reviewed changes

danbev closed this Apr 11, 2019

Uh oh!

Uh oh!

Conversation

apapirovski commented Mar 30, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Mar 30, 2019

Uh oh!

Uh oh!

apapirovski commented Mar 31, 2019

Uh oh!

Uh oh!

Uh oh!

targos commented Mar 31, 2019

Uh oh!

nodejs-github-bot commented Mar 31, 2019

Uh oh!

nodejs-github-bot commented Mar 31, 2019

Uh oh!

Trott commented Mar 31, 2019

Uh oh!

BridgeAR commented Apr 1, 2019

Uh oh!

danbev commented Apr 4, 2019

Uh oh!

BridgeAR commented Apr 4, 2019

Uh oh!

targos commented Apr 4, 2019

Uh oh!

nodejs-github-bot commented Apr 6, 2019

Uh oh!

nodejs-github-bot commented Apr 10, 2019

Uh oh!

danbev commented Apr 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

apapirovski commented Mar 30, 2019•
edited
Loading