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

buffer: improve performance caused by primordials #30235

Closed
wants to merge 3 commits into from

Conversation

@jizusun
Copy link
Contributor

jizusun commented Nov 3, 2019

This is my first try, and it's based on the code-and-learn guidances
This restore some performance after introducing primordialias.

Benchmark compared with master version using the methods mentioned here

Perf Comparison

                                                                                confidence improvement accuracy (*)    (**)   (***)
 buffers/buffer-compare-offset.js n=1000000 size=16 method='offset'                            -2.56 %       ±5.19%  ±6.93%  ±9.07%
 buffers/buffer-compare-offset.js n=1000000 size=16 method='slice'                     ***     17.14 %       ±4.95%  ±6.60%  ±8.60%
 buffers/buffer-compare-offset.js n=1000000 size=16386 method='offset'                         -1.76 %       ±4.77%  ±6.35%  ±8.26%
 buffers/buffer-compare-offset.js n=1000000 size=16386 method='slice'                  ***     21.40 %       ±6.61%  ±8.81% ±11.50%
 buffers/buffer-compare-offset.js n=1000000 size=4096 method='offset'                           1.69 %       ±5.34%  ±7.12%  ±9.28%
 buffers/buffer-compare-offset.js n=1000000 size=4096 method='slice'                   ***     20.82 %       ±5.64%  ±7.52%  ±9.84%
 buffers/buffer-compare-offset.js n=1000000 size=512 method='offset'                           -1.59 %       ±5.90%  ±7.86% ±10.26%
 buffers/buffer-compare-offset.js n=1000000 size=512 method='slice'                    ***     20.01 %       ±4.67%  ±6.22%  ±8.10%

Refs: #29766
Refs: nodejs/code-and-learn#97
Refs: #29633

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
This is my first PR, and it's based on the code-and-learn guidances
This restore some performance after introducing primordialias.

Fixes: #29766
Refs: nodejs/code-and-learn#97
Refs: #29633
@jizusun jizusun mentioned this pull request Nov 3, 2019
2 of 2 tasks complete
@@ -21,7 +21,8 @@

'use strict';

const { Math, Object } = primordials;
const { Object: { defineProperties, defineProperty, setPrototypeOf, create } } = primordials;

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 3, 2019

Member

Did you run make lint? It would usually complain about a line longer than 80 characters.

This comment has been minimized.

Copy link
@jizusun

jizusun Nov 3, 2019

Author Contributor

Good afternoon 😃
Oh what a quick feedback!
Thank you♥️
I'll fix it when I'm back home.

This comment has been minimized.

Copy link
@jizusun

jizusun Nov 3, 2019

Author Contributor

One more thing to ask for help, what's the correct way to do prettify? Normally I have prettier set up with eslint, but I didn't find similar in the Makefile.

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 3, 2019

Member

make lint-js-fix :)

This comment has been minimized.

Copy link
@jizusun

jizusun Nov 4, 2019

Author Contributor

Oh I mean, for the max-len rule, the eslint --fix cannot help.
So I have to correct it by myself, right?

@jizusun

This comment has been minimized.

Copy link
Contributor Author

jizusun commented Nov 3, 2019

I think I also need to include a benchmark section to make my first PR better 😏 , but I don't have time today.
I'll dive deep into it tomorrow.


benchmark attached

lib/buffer.js Outdated Show resolved Hide resolved
@ZYSzys
ZYSzys approved these changes Nov 3, 2019
@trivikr
trivikr approved these changes Nov 3, 2019
Copy link
Member

legendecas left a comment

LGTM % one nit in comments.

lib/buffer.js Outdated Show resolved Hide resolved
@jizusun jizusun force-pushed the jizusun:code-learn-20191103 branch from ba61692 to d04e6d8 Nov 4, 2019
@nodejs-github-bot

This comment has been minimized.

@jizusun

This comment has been minimized.

Copy link
Contributor Author

jizusun commented Nov 4, 2019

Dear all reviewers,

I saw the several builds fails in Jenkins, and one build node-test-commit-freefsd fails because

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

Do you think it's because of my change or the job is not stable (ignore it / re-trigger it)?

@nodejs-github-bot

This comment has been minimized.

@gireeshpunathil

This comment has been minimized.

Copy link
Member

gireeshpunathil commented Nov 4, 2019

@jizusun - we have seen that earlier and reported in #29802 , so don't worry about that.

ZYSzys added a commit that referenced this pull request Nov 4, 2019
This is my first PR, and it's based on the code-and-learn guidances
This restore some performance after introducing primordialias.

Refs: #29766
Refs: nodejs/code-and-learn#97
Refs: #29633

PR-URL: #30235
Refs: #29766
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@ZYSzys

This comment has been minimized.

Copy link
Member

ZYSzys commented Nov 4, 2019

Landed in 22799be.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

@ZYSzys ZYSzys closed this Nov 4, 2019
targos added a commit that referenced this pull request Nov 5, 2019
This is my first PR, and it's based on the code-and-learn guidances
This restore some performance after introducing primordialias.

Refs: #29766
Refs: nodejs/code-and-learn#97
Refs: #29633

PR-URL: #30235
Refs: #29766
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos mentioned this pull request Nov 5, 2019
targos added a commit that referenced this pull request Nov 8, 2019
This is my first PR, and it's based on the code-and-learn guidances
This restore some performance after introducing primordialias.

Refs: #29766
Refs: nodejs/code-and-learn#97
Refs: #29633

PR-URL: #30235
Refs: #29766
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos added a commit that referenced this pull request Nov 10, 2019
This is my first PR, and it's based on the code-and-learn guidances
This restore some performance after introducing primordialias.

Refs: #29766
Refs: nodejs/code-and-learn#97
Refs: #29633

PR-URL: #30235
Refs: #29766
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos added a commit that referenced this pull request Nov 10, 2019
This is my first PR, and it's based on the code-and-learn guidances
This restore some performance after introducing primordialias.

Refs: #29766
Refs: nodejs/code-and-learn#97
Refs: #29633

PR-URL: #30235
Refs: #29766
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos added a commit that referenced this pull request Nov 11, 2019
This is my first PR, and it's based on the code-and-learn guidances
This restore some performance after introducing primordialias.

Refs: #29766
Refs: nodejs/code-and-learn#97
Refs: #29633

PR-URL: #30235
Refs: #29766
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.