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

string_decoder: optimize write() #1209

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Mar 19, 2015

By limiting property getting/setting to only where they are
absolutely necessary, we can achieve greater performance
especially with small utf8 inputs and any size base64 inputs.

ascii/binary are unaffected.

Example results for utf8 and base64 from the included benchmark:

misc/string-decode.js encoding=utf8 inlen=32 chunk=16 n=250000: ./iojs: 1946800 ./iojs-io.js-4655521: 1654400 ........... 17.67%
misc/string-decode.js encoding=utf8 inlen=32 chunk=64 n=250000: ./iojs: 2838500 ./iojs-io.js-4655521: 2545000 ........... 11.53%
misc/string-decode.js encoding=utf8 inlen=32 chunk=256 n=250000: ./iojs: 2822200 ./iojs-io.js-4655521: 2522400 .......... 11.88%
misc/string-decode.js encoding=utf8 inlen=32 chunk=1024 n=250000: ./iojs: 2829100 ./iojs-io.js-4655521: 2507800 ......... 12.81%
misc/string-decode.js encoding=utf8 inlen=128 chunk=16 n=250000: ./iojs: 508720 ./iojs-io.js-4655521: 499830 ............. 1.78%
misc/string-decode.js encoding=utf8 inlen=128 chunk=64 n=250000: ./iojs: 1016400 ./iojs-io.js-4655521: 931340 ............ 9.13%
misc/string-decode.js encoding=utf8 inlen=128 chunk=256 n=250000: ./iojs: 1250200 ./iojs-io.js-4655521: 1179200 .......... 6.02%
misc/string-decode.js encoding=utf8 inlen=128 chunk=1024 n=250000: ./iojs: 1258600 ./iojs-io.js-4655521: 1193400 ......... 5.46%
misc/string-decode.js encoding=utf8 inlen=1024 chunk=16 n=250000: ./iojs: 64944 ./iojs-io.js-4655521: 64852 .............. 0.14%
misc/string-decode.js encoding=utf8 inlen=1024 chunk=64 n=250000: ./iojs: 134380 ./iojs-io.js-4655521: 133250 ............ 0.85%
misc/string-decode.js encoding=utf8 inlen=1024 chunk=256 n=250000: ./iojs: 178810 ./iojs-io.js-4655521: 176190 ........... 1.49%
misc/string-decode.js encoding=utf8 inlen=1024 chunk=1024 n=250000: ./iojs: 215380 ./iojs-io.js-4655521: 212140 .......... 1.52%
misc/string-decode.js encoding=base64-utf8 inlen=32 chunk=16 n=250000: ./iojs: 407580 ./iojs-io.js-4655521: 307650 ...... 32.48%
misc/string-decode.js encoding=base64-utf8 inlen=32 chunk=64 n=250000: ./iojs: 1422000 ./iojs-io.js-4655521: 1120100 .... 26.96%
misc/string-decode.js encoding=base64-utf8 inlen=32 chunk=256 n=250000: ./iojs: 1416700 ./iojs-io.js-4655521: 1160400 ... 22.09%
misc/string-decode.js encoding=base64-utf8 inlen=32 chunk=1024 n=250000: ./iojs: 1339300 ./iojs-io.js-4655521: 1140600 .. 17.42%
misc/string-decode.js encoding=base64-utf8 inlen=128 chunk=16 n=250000: ./iojs: 93840 ./iojs-io.js-4655521: 84048 ....... 11.65%
misc/string-decode.js encoding=base64-utf8 inlen=128 chunk=64 n=250000: ./iojs: 383150 ./iojs-io.js-4655521: 308310 ..... 24.28%
misc/string-decode.js encoding=base64-utf8 inlen=128 chunk=256 n=250000: ./iojs: 1067600 ./iojs-io.js-4655521: 886690 ... 20.41%
misc/string-decode.js encoding=base64-utf8 inlen=128 chunk=1024 n=250000: ./iojs: 1011700 ./iojs-io.js-4655521: 926400 ... 9.21%
misc/string-decode.js encoding=base64-utf8 inlen=1024 chunk=16 n=250000: ./iojs: 13682 ./iojs-io.js-4655521: 10591 ...... 29.19%
misc/string-decode.js encoding=base64-utf8 inlen=1024 chunk=64 n=250000: ./iojs: 48976 ./iojs-io.js-4655521: 39895 ...... 22.76%
misc/string-decode.js encoding=base64-utf8 inlen=1024 chunk=256 n=250000: ./iojs: 141090 ./iojs-io.js-4655521: 120230 ... 17.35%
misc/string-decode.js encoding=base64-utf8 inlen=1024 chunk=1024 n=250000: ./iojs: 269880 ./iojs-io.js-4655521: 244070 .. 10.57%
misc/string-decode.js encoding=base64-ascii inlen=32 chunk=16 n=250000: ./iojs: 497550 ./iojs-io.js-4655521: 396600 ..... 25.45%
misc/string-decode.js encoding=base64-ascii inlen=32 chunk=64 n=250000: ./iojs: 1521700 ./iojs-io.js-4655521: 1174800 ... 29.52%
misc/string-decode.js encoding=base64-ascii inlen=32 chunk=256 n=250000: ./iojs: 1487500 ./iojs-io.js-4655521: 1174100 .. 26.69%
misc/string-decode.js encoding=base64-ascii inlen=32 chunk=1024 n=250000: ./iojs: 1401600 ./iojs-io.js-4655521: 1155800 . 21.27%
misc/string-decode.js encoding=base64-ascii inlen=128 chunk=16 n=250000: ./iojs: 142030 ./iojs-io.js-4655521: 114770 .... 23.75%
misc/string-decode.js encoding=base64-ascii inlen=128 chunk=64 n=250000: ./iojs: 499170 ./iojs-io.js-4655521: 396290 .... 25.96%
misc/string-decode.js encoding=base64-ascii inlen=128 chunk=256 n=250000: ./iojs: 1145200 ./iojs-io.js-4655521: 999490 .. 14.57%
misc/string-decode.js encoding=base64-ascii inlen=128 chunk=1024 n=250000: ./iojs: 1207200 ./iojs-io.js-4655521: 983240 . 22.77%
misc/string-decode.js encoding=base64-ascii inlen=1024 chunk=16 n=250000: ./iojs: 18163 ./iojs-io.js-4655521: 14729 ..... 23.31%
misc/string-decode.js encoding=base64-ascii inlen=1024 chunk=64 n=250000: ./iojs: 69888 ./iojs-io.js-4655521: 60726 ..... 15.09%
misc/string-decode.js encoding=base64-ascii inlen=1024 chunk=256 n=250000: ./iojs: 194780 ./iojs-io.js-4655521: 141810 .. 37.36%
misc/string-decode.js encoding=base64-ascii inlen=1024 chunk=1024 n=250000: ./iojs: 356430 ./iojs-io.js-4655521: 342270 .. 4.14%

By limiting property getting/setting to only where they are
absolutely necessary, we can achieve greater performance
especially with small utf8 inputs and any size base64 inputs.
@rvagg
Copy link
Member

rvagg commented Mar 19, 2015

nice, I like those utf8 numbers, any idea how much property getting is impacting this?

LGTM

@mscdex
Copy link
Contributor Author

mscdex commented Mar 19, 2015

I made these changes awhile ago so I can't recall for sure, but this patch is almost entirely made up of changes to avoid property lookups, so I would say that it is impacting things the most (especially in the loop in write()) versus some of the other minor non-property-related changes.

@mscdex
Copy link
Contributor Author

mscdex commented Mar 20, 2015

/cc @iojs/collaborators

@micnic
Copy link
Contributor

micnic commented Mar 20, 2015

LGTM

@petkaantonov
Copy link
Contributor

This should be implemented in C, we will need utf8 decoder in the future for buffer.toString() which is currently delegating to V8's utf8 decoder.

@mscdex
Copy link
Contributor Author

mscdex commented Mar 20, 2015

@petkaantonov Why? Is V8 dropping their UTF-8 decoder soon?

@petkaantonov
Copy link
Contributor

@mscdex No but instead of using NewFromUtf8 (which v8 internally decodes and copies to jsheap), we would decode it ourselves into an external buffer which doesn't take space in jsheap and also allows string sizes bigger than 290mb or so. See #649

// determine how many bytes we have to check at the end of this buffer
var i = (buffer.length >= 3) ? 3 : buffer.length;
var i = (buflen >= 3) ? 3 : buflen;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if V8 will unroll the subsequent loop (or if there's value in doing so ourselves.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean for the i = 3 case? I didn't try that since I figured people would frown about code duplication between the i = 3 and i = buflen cases. I also did not see what kind performance difference it would make though.

@chrisdickinson
Copy link
Contributor

This LGTM.

This should be implemented in C, we will need utf8 decoder in the future for buffer.toString() which is currently delegating to V8's utf8 decoder.

We'll still have to support this module if we move to a C decoder, so making it faster is probably worthwhile.

@mscdex mscdex added the string_decoder Issues and PRs related to the string_decoder subsystem. label Mar 25, 2015
mscdex added a commit that referenced this pull request Mar 25, 2015
By limiting property getting/setting to only where they are
absolutely necessary, we can achieve greater performance
especially with small utf8 inputs and any size base64 inputs.

PR-URL: #1209
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Nicu Micleușanu <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
@mscdex
Copy link
Contributor Author

mscdex commented Mar 25, 2015

Landed in 8a94581.

@mscdex mscdex closed this Mar 25, 2015
@mscdex mscdex deleted the perf-stringdecoder branch March 25, 2015 04:39
@rvagg rvagg mentioned this pull request Mar 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
string_decoder Issues and PRs related to the string_decoder subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants