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.

Sign up for GitHub

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

Jump to bottom

buffer: implement iterable interface #525

Closed
wants to merge 3 commits into from
Closed

buffer: implement iterable interface #525

wants to merge 3 commits into from

Conversation

vkurchatkin
Copy link
Contributor

@vkurchatkin
buffer: implement iterable interface
47a5310 
This makes possible to use `for..of` loop with
buffers. Also related `keys`, `values` and `entries`
methods are added for feature parity with `Uint8Array`.
@vkurchatkin vkurchatkin mentioned this pull request Jan 20, 2015
Closed
@vkurchatkin
Copy link
Contributor Author

Benchmark results:

buffers/buffer-iterate.js size=16 type=fast method=for n=1000: 317787
buffers/buffer-iterate.js size=16 type=fast method=forOf n=1000: 709499
buffers/buffer-iterate.js size=16 type=slow method=for n=1000: 1252185
buffers/buffer-iterate.js size=16 type=slow method=forOf n=1000: 782374
buffers/buffer-iterate.js size=512 type=fast method=for n=1000: 740854
buffers/buffer-iterate.js size=512 type=fast method=forOf n=1000: 67528
buffers/buffer-iterate.js size=512 type=slow method=for n=1000: 522403
buffers/buffer-iterate.js size=512 type=slow method=forOf n=1000: 60941
buffers/buffer-iterate.js size=1024 type=fast method=for n=1000: 491065
buffers/buffer-iterate.js size=1024 type=fast method=forOf n=1000: 34168
buffers/buffer-iterate.js size=1024 type=slow method=for n=1000: 557932
buffers/buffer-iterate.js size=1024 type=slow method=forOf n=1000: 27384
buffers/buffer-iterate.js size=4096 type=fast method=for n=1000: 233263
buffers/buffer-iterate.js size=4096 type=fast method=forOf n=1000: 8853
buffers/buffer-iterate.js size=4096 type=slow method=for n=1000: 148115
buffers/buffer-iterate.js size=4096 type=slow method=forOf n=1000: 8643
buffers/buffer-iterate.js size=16386 type=fast method=for n=1000: 53974
buffers/buffer-iterate.js size=16386 type=fast method=forOf n=1000: 2312
buffers/buffer-iterate.js size=16386 type=slow method=for n=1000: 65763
buffers/buffer-iterate.js size=16386 type=slow method=forOf n=1000: 2175

@vkurchatkin
Copy link
Contributor Author

Basically it's 30 times slower. Not that bad. Eventually will become faster: https://code.google.com/p/v8/issues/detail?id=3822

Looks like functions with for..of cannot be optimized at the moment:

[disabled optimization for 0x383853e26119 <SharedFunctionInfo stuff>, reason: ForOfStatement]

@rvagg
Copy link
Member

rvagg commented Jan 20, 2015

Another one for 1.1.0 perhaps

bnoordhuis
bnoordhuis reviewed Jan 20, 2015
View reviewed changes
test/parallel/test-buffer-iterator.js
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.
Copy link
Member

Choose a reason for hiding this comment

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

@vkurchatkin Can you drop the license boilerplate? The TC recently decided to do away with it.

@bnoordhuis
Copy link
Member

LGTM but like @rvagg said, on hold for 1.1. The strategy for minor version bumps will be discussed in tomorrow's TC meeting. But thanks for doing this, Vladimir!

@feross
Copy link
Contributor

feross commented Jan 20, 2015

@vkurchatkin If you're feeling ambitious, please also send a PR to the npm module buffer so browserify users get these same improvements too!

@vkurchatkin
Copy link
Contributor Author

@feross I'll look into it, but a quick test shows that it works already in Chrome since Uint8Array is iterable.

@vkurchatkin
Copy link
Contributor Author

added one more benchmark case which use iterator, but not for..of:

var iter = buffer[Symbol.iterator]();
var cur = iter.next();

while (!cur.done) {
  console.log(cur.value);
  cur = iter.next();
}

Results:

buffers/buffer-iterate.js size=16 type=fast method=for n=1000: 1234429
buffers/buffer-iterate.js size=16 type=fast method=forOf n=1000: 689413
buffers/buffer-iterate.js size=16 type=fast method=iterator n=1000: 704994
buffers/buffer-iterate.js size=16 type=slow method=for n=1000: 1037244
buffers/buffer-iterate.js size=16 type=slow method=forOf n=1000: 763083
buffers/buffer-iterate.js size=16 type=slow method=iterator n=1000: 648382
buffers/buffer-iterate.js size=512 type=fast method=for n=1000: 761231
buffers/buffer-iterate.js size=512 type=fast method=forOf n=1000: 67708
buffers/buffer-iterate.js size=512 type=fast method=iterator n=1000: 260788
buffers/buffer-iterate.js size=512 type=slow method=for n=1000: 548060
buffers/buffer-iterate.js size=512 type=slow method=forOf n=1000: 65271
buffers/buffer-iterate.js size=512 type=slow method=iterator n=1000: 259855
buffers/buffer-iterate.js size=1024 type=fast method=for n=1000: 418621
buffers/buffer-iterate.js size=1024 type=fast method=forOf n=1000: 34951
buffers/buffer-iterate.js size=1024 type=fast method=iterator n=1000: 182922
buffers/buffer-iterate.js size=1024 type=slow method=for n=1000: 457019
buffers/buffer-iterate.js size=1024 type=slow method=forOf n=1000: 34288
buffers/buffer-iterate.js size=1024 type=slow method=iterator n=1000: 191319
buffers/buffer-iterate.js size=4096 type=fast method=for n=1000: 232112
buffers/buffer-iterate.js size=4096 type=fast method=forOf n=1000: 9086
buffers/buffer-iterate.js size=4096 type=fast method=iterator n=1000: 70217
buffers/buffer-iterate.js size=4096 type=slow method=for n=1000: 206603
buffers/buffer-iterate.js size=4096 type=slow method=forOf n=1000: 8845
buffers/buffer-iterate.js size=4096 type=slow method=iterator n=1000: 68734
buffers/buffer-iterate.js size=16386 type=fast method=for n=1000: 66488
buffers/buffer-iterate.js size=16386 type=fast method=forOf n=1000: 2094
buffers/buffer-iterate.js size=16386 type=fast method=iterator n=1000: 19092
buffers/buffer-iterate.js size=16386 type=slow method=for n=1000: 65570
buffers/buffer-iterate.js size=16386 type=slow method=forOf n=1000: 2210
buffers/buffer-iterate.js size=16386 type=slow method=iterator n=1000: 19635

I think we can expect at least as much speed with for..of in the future

@rvagg
Copy link
Member

rvagg commented Jan 23, 2015

this was tagged with tc-agenda but I'm not sure we properly covered it, @bnoordhuis your call as to whether the tag is removed or left for next meeting bundled with further versioning discussion

@rvagg rvagg mentioned this pull request Jan 23, 2015
Closed
@rvagg rvagg added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 23, 2015
@rvagg
Copy link
Member

rvagg commented Jan 27, 2015

#625 being merged has forced us to a semver-minor situation so this could probably be merged now so it makes it in to 1.1.0, I don't see any -1s here.

@timoxley timoxley mentioned this pull request Jan 28, 2015
Closed
vkurchatkin added a commit that referenced this pull request Jan 28, 2015
@vkurchatkin
buffer: implement iterable interface
45d8d9f 
This makes possible to use `for..of` loop with
buffers. Also related `keys`, `values` and `entries`
methods are added for feature parity with `Uint8Array`.

PR-URL: #525
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@vkurchatkin
Copy link
Contributor Author

landed in 45d8d9f

@rvagg rvagg removed the tc-agenda label Feb 3, 2015
@vkurchatkin vkurchatkin mentioned this pull request Feb 9, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vkurchatkin @rvagg @bnoordhuis @feross