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: little improve for Buffer.concat method #1437

Closed
wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

When buffer list less than 2, no need to calculate the length.
The change's benchmark result is here:
https://gist.github.com/JacksonTian/2c9e2bdec00018e010e6
It improve 15% ~ 25% speed when list only have one buffer,
to other cases no effect.

When buffer list less than 2, no need to calculate the length.
The change's benchmark result is here:
https://gist.github.com/JacksonTian/2c9e2bdec00018e010e6
It improve 15% ~ 25% speed when list only have one buffer,
to other cases no effect.
@brendanashworth brendanashworth added the buffer Issues and PRs related to the buffer subsystem. label Apr 16, 2015
if (list.length === 0)
return new Buffer(0);
else if (list.length === 1)
return list[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, should we follow the below assertions:

assert.equal(Buffer.cancat(anyArray, len).length, len);

however

assert.equal(Buffer.cancat([], 1).length, 1); // this line would fail
assert.equal(Buffer.concat([new Buffer(20)]).length, 10); this line would fail as well and importantly i think we need to cut the buffer to 10.

@JacksonTian I know this should not be related to your patch, but it would be great if this is able to be fixed(bug? unsure) :p though cc @trevnorris

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yorkie for bringing that up. Let's address that issue in another PR and keep this one simple.

@fengmk2
Copy link
Contributor

fengmk2 commented Apr 16, 2015

LGTM

@trevnorris
Copy link
Contributor

LGTM. Thanks.

brendanashworth pushed a commit that referenced this pull request Apr 23, 2015
When buffer list less than 2, no need to calculate the length.
The change's benchmark result is here:
https://gist.github.com/JacksonTian/2c9e2bdec00018e010e6
It improve 15% ~ 25% speed when list only have one buffer,
to other cases no effect.

PR-URL: #1437
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
@brendanashworth
Copy link
Contributor

LGTM.

Thanks @JacksonTian, merged in 3d3083b!

@rvagg rvagg mentioned this pull request Apr 27, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
When buffer list less than 2, no need to calculate the length.
The change's benchmark result is here:
https://gist.github.com/JacksonTian/2c9e2bdec00018e010e6
It improve 15% ~ 25% speed when list only have one buffer,
to other cases no effect.

PR-URL: nodejs#1437
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
@ChALkeR ChALkeR added the performance Issues and PRs related to the performance of Node.js. label Feb 16, 2016
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. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants