Details
-
Bug
-
Status: Closed
-
Critical
-
Resolution: Fixed
-
0.11.0
-
None
-
Tested in Chrome 67.0.3396.87 and Safari 11.1.1 (13605.2.8) on OSX.
-
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
Attachments
Attachments
Issue Links
- links to