[jira] [Commented] (THRIFT-4592) JS: readI32 performance on large arrays is very poor in Chrome

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/THRIFT-4592?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16525346#comment-16525346 ] 

ASF GitHub Bot commented on THRIFT-4592:
----------------------------------------

arittr opened a new 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
 
 
   THRIFT-4592: [JS] change readI32 to use Array.reverse/Array.pop vs Array.shift, which is expensive for big arrays in V8
   
   `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.
   
   Some helpful tips for a successful Apache Thrift PR:
   
   * Did you test your changes locally or using CI in your fork?
   * Is the Apache Jira THRIFT ticket identifier in the PR title?
   * Is the Apache Jira THRIFT ticket identifier in the commit message?
   * Did you squash your changes to a single commit?
   * Are these changes backwards compatible? (please say so in PR description)
   * Do you need to update the language-specific README?
   
   Example ideal pull request title:
   
           THRIFT-9999: an example pull request title
   
   Example ideal commit message:
   
           THRIFT-9999: [summary of fix, one line if possible]
           Client: [language(s) affected, comma separated, use lib/ directory names please]
   
   For more information about committing, see CONTRIBUTING.md
   

----------------------------------------------------------------
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)