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: fix performance regression #5134

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Feb 7, 2016

This commit reverts the const usage introduced by 68a6abc because v8 currently cannot optimize functions that contain these uses of const (unsupported phi use of const variable). The performance difference in this case can be up to ~130% for non-ascii/binary string encodings.

@mscdex mscdex added the string_decoder Issues and PRs related to the string_decoder subsystem. label Feb 7, 2016
@bnoordhuis
Copy link
Member

I'd split the revert and the optimization into separate commits but otherwise LGTM.

I'm kind of surprised V8 balks at it. Was that with 4.6 or 4.8?

@mscdex
Copy link
Contributor Author

mscdex commented Feb 7, 2016

I tested on the master branch (4.8.271).

@mscdex
Copy link
Contributor Author

mscdex commented Feb 7, 2016

@jasnell
Copy link
Member

jasnell commented Feb 10, 2016

this is a bit unfortunate but LGTM. One suggestion tho: it would be good to add some code comments that indicates what was reverted and why (with a reference to the V8 version). That would allow someone to come back later and revisit.

@mscdex
Copy link
Contributor Author

mscdex commented Feb 10, 2016

I'm not sure that adding code comments in this particular situation would be very useful and/or feasible considering this is a much more general issue that can apply almost anywhere in the code base.

@jasnell
Copy link
Member

jasnell commented Feb 10, 2016

Ok, that's fine. Still LGTM

This commit reverts the const usage introduced by 68a6abc
because v8 currently cannot optimize functions that contain
these uses of const (unsupported phi use of const variable).
The performance difference in this case can be up to ~130%
for non-ascii/binary string encodings.
@mscdex mscdex force-pushed the fix-string-decoder-write-deopt branch from e58ae86 to 67edf11 Compare February 11, 2016 15:40
mscdex added a commit that referenced this pull request Feb 11, 2016
This commit reverts the const usage introduced by 68a6abc
because v8 currently cannot optimize functions that contain
these uses of const (unsupported phi use of const variable).
The performance difference in this case can be up to ~130%
for non-ascii/binary string encodings.

PR-URL: #5134
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mscdex
Copy link
Contributor Author

mscdex commented Feb 11, 2016

Landed in ae244a2.

@mscdex mscdex closed this Feb 11, 2016
@mscdex mscdex deleted the fix-string-decoder-write-deopt branch February 11, 2016 15:45
rvagg pushed a commit that referenced this pull request Feb 15, 2016
This commit reverts the const usage introduced by 68a6abc
because v8 currently cannot optimize functions that contain
these uses of const (unsupported phi use of const variable).
The performance difference in this case can be up to ~130%
for non-ascii/binary string encodings.

PR-URL: #5134
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
stefanmb pushed a commit to stefanmb/node that referenced this pull request Feb 23, 2016
This commit reverts the const usage introduced by 68a6abc
because v8 currently cannot optimize functions that contain
these uses of const (unsupported phi use of const variable).
The performance difference in this case can be up to ~130%
for non-ascii/binary string encodings.

PR-URL: nodejs#5134
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

As 68a6abc landed on LTS I think this should likely as well. Thoughts?

@jasnell
Copy link
Member

jasnell commented Mar 11, 2016

SGTM

@rvagg
Copy link
Member

rvagg commented Mar 14, 2016

lgtm, very minor change, (near) zero edge-case potential

MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
This commit reverts the const usage introduced by 68a6abc
because v8 currently cannot optimize functions that contain
these uses of const (unsupported phi use of const variable).
The performance difference in this case can be up to ~130%
for non-ascii/binary string encodings.

PR-URL: #5134
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
This commit reverts the const usage introduced by 68a6abc
because v8 currently cannot optimize functions that contain
these uses of const (unsupported phi use of const variable).
The performance difference in this case can be up to ~130%
for non-ascii/binary string encodings.

PR-URL: #5134
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
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