string_decoder: fix performance regression - #5134
Conversation
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
I tested on the master branch (4.8.271). |
Sorry, something went wrong.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Ok, that's fine. Still LGTM |
Sorry, something went wrong.
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.
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>
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>
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>
|
As 68a6abc landed on LTS I think this should likely as well. Thoughts? |
Sorry, something went wrong.
|
SGTM |
Sorry, something went wrong.
|
lgtm, very minor change, (near) zero edge-case potential |
Sorry, something went wrong.
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>
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>
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.