You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "ASF GitHub Bot (JIRA)" <ji...@apache.org> on 2018/07/03 12:16:00 UTC
[jira] [Commented] (THRIFT-4592) JS: readI32 performance on large
arrays is very poor in Chrome
[ https://issues.apache.org/jira/browse/THRIFT-4592?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16531280#comment-16531280 ]
ASF GitHub Bot commented on THRIFT-4592:
----------------------------------------
jeking3 closed pull request #1569: THRIFT-4592: [JS] change readI32 to use Array.reverse/Array.pop vs Array.shift
URL: https://github.com/apache/thrift/pull/1569
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/lib/js/src/thrift.js b/lib/js/src/thrift.js
index 2b385a3a71..39e6db19bc 100644
--- a/lib/js/src/thrift.js
+++ b/lib/js/src/thrift.js
@@ -1319,7 +1319,11 @@ Thrift.Protocol.prototype = {
if (f.length === 0) {
r.value = undefined;
} else {
- r.value = f.shift();
+ if (!f.isReversed) {
+ f.reverse();
+ f.isReversed = true;
+ }
+ r.value = f.pop();
}
} else if (f instanceof Object) {
for (var i in f) {
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
> JS: readI32 performance on large arrays is very poor in Chrome
> --------------------------------------------------------------
>
> Key: THRIFT-4592
> URL: https://issues.apache.org/jira/browse/THRIFT-4592
> Project: Thrift
> Issue Type: Bug
> Components: JavaScript - Library
> Affects Versions: 0.11.0
> Environment: Tested in Chrome 67.0.3396.87 and Safari 11.1.1 (13605.2.8) on OSX.
> Reporter: Drew Ritter
> Priority: Critical
> Attachments: ThriftBenchmarker.js, readI32_pop.patch
>
>
> `readI32` ( [https://github.com/apache/thrift/blob/master/lib/js/src/thrift.js#L1311-L1341] ) performance is extremely poor in Chrome when reading values from large arrays. This is due to the use of Array.shift ( [https://github.com/apache/thrift/blob/master/lib/js/src/thrift.js#L1322] ) which seems to have performance issues in V8/Chrome.
>
> As Chrome is a high market share browser I propose either changing this to use reverse/pop or detect the appropriate strategy given array length. I think this is due to how V8 handles large arrays and the heuristics used to determine whether data should be stored in a C-backed array or hash map.
>
> STR:
> 0) Download or copy the attached ThriftBenchmarker.js - this file has some helper functions to build large arrays, as well as a copy/paste of the current `readI32` as well as one that uses Array.reverse/Array.pop over Array.shift.
> 1) Run this test in Chrome - you can simply copy/paste the code into the Console.
> 2) Observe that the tests using Array.shift are quite slow in Chrome:
> {code:java}
> -- testing long arrays --
> shift test: 1084.566162109375ms
> pop test: 37.604248046875ms
> -- testing long arrays of strings --
> shift test: 5189.416015625ms
> pop test: 17.9189453125ms
> -- testing short arrays --
> shift test: 0.06005859375ms
> pop test: 0.016845703125ms
> -- testing short arrays of strings --
> shift test: 0.01611328125ms
> pop test: 0.01318359375ms{code}
> 3) Run this test in Safari (or a non-V8) browser.
> 4) Observe that the tests using Array.shift are far faster:
> {code:java}
> -- testing long arrays --
> shift test: 19.235ms
> pop test: 11.774ms
> - testing long arrays of strings --
> shift test: 13.087ms
> pop test: 12.805ms
> - testing short arrays --
> shift test: 0.049ms
> pop test: 0.054ms
> - testing short arrays of strings --
> shift test: 0.039ms
> pop test: 0.022ms{code}
> [^ThriftBenchmarker.js]
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)