Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-4592

JS: readI32 performance on large arrays is very poor in Chrome

VotersWatch issueWatchersLinkCloneUpdate Comment AuthorReplace String in CommentUpdate Comment VisibilityDelete Comments
    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 0.11.0
    • Fix Version/s: 0.12.0
    • Component/s: JavaScript - Library
    • Labels:
      None
    • Environment:

      Tested in Chrome 67.0.3396.87 and Safari 11.1.1 (13605.2.8) on OSX.

    • Patch Info:
      Patch Available

      Description

      `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:

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

      3) Run this test in Safari (or a non-V8) browser.

      4) Observe that the tests using Array.shift are far faster:

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

      ThriftBenchmarker.js

        Attachments

        1. ThriftBenchmarker.js
          5 kB
          Drew Ritter
        2. readI32_pop.patch
          0.6 kB
          Drew Ritter

          Activity

            People

            • Assignee:
              jking3 James E. King III
              Reporter:
              arittr Drew Ritter

              Dates

              • Created:
                Updated:
                Resolved:

                Issue deployment