Optimize new TypedArray(typedArray) constructor

src/js/typedarray.js

 // Copyright 2013 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 (function(global, utils) {
 
 "use strict";
 
 %CheckIsBootstrapping();
 
@@ skipping 184 matching lines @@
   if (byteLength > %_TypedArrayMaxSizeInHeap()) {
     var buffer = new GlobalArrayBuffer(byteLength);
     %_TypedArrayInitialize(obj, ARRAY_ID, buffer, 0, byteLength, true);
   } else {
     %_TypedArrayInitialize(obj, ARRAY_ID, null, 0, byteLength, true);
   }
 }
 
 function NAMEConstructByArrayLike(obj, arrayLike) {
   var length = arrayLike.length;
+
   var l = ToPositiveInteger(length, kInvalidTypedArrayLength);
 
   if (l > %_MaxSmi()) {
     throw MakeRangeError(kInvalidTypedArrayLength);
   }
-  var initialized = false;
   var byteLength = l * ELEMENT_SIZE;
-  if (byteLength <= %_TypedArrayMaxSizeInHeap()) {

[adamk, 2016/03/08 19:32:10]

What's up with this if statement? Why is it OK to remove it, that is?

[Dan Ehrenberg, 2016/03/08 19:55:55]

Apparently it wasn't, and this was found by the tests! The new version leaves
this function as-is, except for getting length passed in from the outside. I
thought somehow it only mattered for TypedArrays, but it matters for arraylikes
also.

-    %_TypedArrayInitialize(obj, ARRAY_ID, null, 0, byteLength, false);
-  } else {
-    initialized =
-        %TypedArrayInitializeFromArrayLike(obj, ARRAY_ID, arrayLike, l);
-  }
-  if (!initialized) {
-    for (var i = 0; i < l; i++) {
-      // It is crucial that we let any execptions from arrayLike[i]
-      // propagate outside the function.
-      obj[i] = arrayLike[i];
-    }
+  %_TypedArrayInitialize(obj, ARRAY_ID, null, 0, byteLength, false);
+  for (var i = 0; i < l; i++) {
+    // It is crucial that we let any exceptions from arrayLike[i]

[adamk, 2016/03/08 19:32:10]

Not sure this comment is worth keeping around.

[Dan Ehrenberg, 2016/03/08 19:55:54]

How about I leave all this as is for now, since I'm not changing this part
anymore, and come back later when I do the backlog of TypedArray cleanups I've
been thinking about?

[adamk, 2016/03/08 20:05:39]

Fine to leave for now since you're not touching it anymore.

+    // propagate outside the function.
+    obj[i] = arrayLike[i];
   }
 }
 
 function NAMEConstructByIterable(obj, iterable, iteratorFn) {
   var list = new InternalArray();
   // Reading the Symbol.iterator property of iterable twice would be
   // observable with getters, so instead, we call the function which
   // was already looked up, and wrap it in another iterable. The
   // __proto__ of the new iterable is set to null to avoid any chance
   // of modifications to Object.prototype being observable here.
@@ skipping 10 matching lines @@
   NAMEConstructByArrayLike(obj, list);
 }
 
 // ES#sec-typedarray-typedarray TypedArray ( typedArray )
 function NAMEConstructByTypedArray(obj, typedArray) {
   // TODO(littledan): Throw on detached typedArray
   var srcData = %TypedArrayGetBuffer(typedArray);
   var length = %_TypedArrayGetLength(typedArray);
   var byteLength = %_ArrayBufferViewGetByteLength(typedArray);
   var newByteLength = length * ELEMENT_SIZE;
+  var initialized = %TypedArrayInitializeFromArrayLike(obj, ARRAY_ID, typedArray, length);

[adamk, 2016/03/08 19:32:10]

Will this ever return false? If not, and this is the only remaining caller,
should it be changed to %TypedArrayInitializeFromTypedArray?

Also, 80 col nit.

[Dan Ehrenberg, 2016/03/08 19:55:54]

I played around with a bunch of alternatives here, including changing around
what the runtime function does. I ended up reverting to a more conservative path
once I realized how the previous code was taking advantage of the runtime
function in the arraylike case (not redundantly zeroing and initializing, which
is OK because an uninitialized ArrayBuffer never escapes, even if an exception
is thrown when setting properties).

   var bufferConstructor = SpeciesConstructor(srcData, GlobalArrayBuffer);
-  var data = new GlobalArrayBuffer(newByteLength);
   var prototype = bufferConstructor.prototype;
   // TODO(littledan): Use the right prototype based on bufferConstructor's realm
   if (IS_RECEIVER(prototype) && prototype !== GlobalArrayBufferPrototype) {
-    %InternalSetPrototype(data, prototype);
+    %InternalSetPrototype(%TypedArrayGetBuffer(obj), prototype);
   }
-  %_TypedArrayInitialize(obj, ARRAY_ID, data, 0, newByteLength, true);
-  // Note: The separate CloneArrayBuffer path in the spec ensures
-  // that NaNs are not canonicalized, but because V8 does not
-  // canonicalize NaNs, we do not have to do anything different.
-  // TODO(littledan): Make a fastpath based on memcpy
-  for (var i = 0; i < length; i++) {
-    obj[i] = typedArray[i];
+  if (!initialized) {
+    for (var i = 0; i < length; i++) {
+      obj[i] = typedArray[i];
+    }
   }
 }
 
 function NAMEConstructor(arg1, arg2, arg3) {
   if (!IS_UNDEFINED(new.target)) {
     if (IS_ARRAYBUFFER(arg1) || IS_SHAREDARRAYBUFFER(arg1)) {
       NAMEConstructByArrayBuffer(this, arg1, arg2, arg3);
     } else if (IS_NUMBER(arg1) || IS_STRING(arg1) ||
                IS_BOOLEAN(arg1) || IS_UNDEFINED(arg1)) {
       NAMEConstructByLength(this, arg1);
@@ skipping 728 matching lines @@
   "setUint32", DataViewSetUint32JS,
 
   "getFloat32", DataViewGetFloat32JS,
   "setFloat32", DataViewSetFloat32JS,
 
   "getFloat64", DataViewGetFloat64JS,
   "setFloat64", DataViewSetFloat64JS
 ]);
 
 })