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.length performance regressed in 6.5.0 #9006

Closed
Nibbler999 opened this Issue Oct 10, 2016 · 9 comments

Comments

Projects
None yet
7 participants
@Nibbler999
Contributor

Nibbler999 commented Oct 10, 2016

  • Version: v6.7.0
  • Platform: Linux x86_64
  • Subsystem: Buffer
[nibbler@nibbler ~]$ cat buffer.js 
'use strict';

var data = new Buffer(1024);

console.time('test');

for (var i = 0; i < 1000000; i++) {
    for (var j = 0; j < data.length; j++) {

    }
}

console.timeEnd('test');

[nibbler@nibbler ~]$ ~/Downloads/node-v6.4.0-linux-x64/bin/node buffer.js 
test: 623.471ms
[nibbler@nibbler ~]$ ~/Downloads/node-v6.5.0-linux-x64/bin/node buffer.js 
test: 2837.938ms
@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Oct 10, 2016

Contributor

I think about the only change that would have affected that was the V8 5.0->5.1 upgrade.

EDIT: FWIW v6.4.0 and master have the same results, but I can confirm that v6.5.0 is a little over twice as slow when running that test.

Contributor

mscdex commented Oct 10, 2016

I think about the only change that would have affected that was the V8 5.0->5.1 upgrade.

EDIT: FWIW v6.4.0 and master have the same results, but I can confirm that v6.5.0 is a little over twice as slow when running that test.

@addaleax addaleax added the V8 Engine label Oct 10, 2016

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Oct 10, 2016

Contributor

/cc @nodejs/v8 ?

Contributor

mscdex commented Oct 10, 2016

/cc @nodejs/v8 ?

@targos targos self-assigned this Oct 10, 2016

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Oct 10, 2016

Member

I think the fix would be v8/v8@3c927e0 but the diff is huge.

Member

targos commented Oct 10, 2016

I think the fix would be v8/v8@3c927e0 but the diff is huge.

@jeisinger

This comment has been minimized.

Show comment
Hide comment
Contributor

jeisinger commented Oct 11, 2016

@bmeurer

This comment has been minimized.

Show comment
Hide comment
@bmeurer

bmeurer Oct 11, 2016

Member

Yeah, I guess it would be fixed by v8/v8@3c927e0.

Member

bmeurer commented Oct 11, 2016

Yeah, I guess it would be fixed by v8/v8@3c927e0.

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Oct 19, 2017

Member

ping @targos @bmeurer — do you know if this was ever addressed in v6.x?

Member

apapirovski commented Oct 19, 2017

ping @targos @bmeurer — do you know if this was ever addressed in v6.x?

@addaleax addaleax added the v6.x label Oct 19, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Oct 19, 2017

Member

@apapirovski I can reproduce this using the latest v6.x release (6.11.4), so I’d assume “no”. It might be something we’d just have to accept as a wontfix if nobody thinks they can put in the work to fix it?

Member

addaleax commented Oct 19, 2017

@apapirovski I can reproduce this using the latest v6.x release (6.11.4), so I’d assume “no”. It might be something we’d just have to accept as a wontfix if nobody thinks they can put in the work to fix it?

@addaleax addaleax removed the buffer label Oct 19, 2017

@bmeurer

This comment has been minimized.

Show comment
Hide comment
@bmeurer

bmeurer Oct 19, 2017

Member

This was never addressed for v6.x, no. Not sure it's worth spending time on this, esp. since this code is fairly different in V8 nowadays.

Member

bmeurer commented Oct 19, 2017

This was never addressed for v6.x, no. Not sure it's worth spending time on this, esp. since this code is fairly different in V8 nowadays.

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Apr 11, 2018

Member

Given the feedback last time and the lack of any movement, I'm going to go ahead and close. If anyone feels like they want to take it on or this is an important issue, feel free to reopen.

Member

apapirovski commented Apr 11, 2018

Given the feedback last time and the lack of any movement, I'm going to go ahead and close. If anyone feels like they want to take it on or this is an important issue, feel free to reopen.

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