ES6 Fix TypedArray constructors.
Created attachment 262922 [details] Patch
Comment on attachment 262922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262922&action=review Looks good overall. I think I found some bugs. > Source/JavaScriptCore/builtins/TypedArrayConstructor.js:37 > + throw new @TypeError("TypedArray.from requires its this argument subclass a TypedArray constructor"); to subclass? be a subclass of? > Source/JavaScriptCore/builtins/TypedArrayConstructor.js:51 > + let mapFn = arguments.length > 1 ? arguments[1] : undefined; This is the same as arguments[1]. > Source/JavaScriptCore/builtins/TypedArrayConstructor.js:53 > + let thisArg; It seems like this function will end up reading thisArg uninitialized (TDZ). You should fix this and add a test. > Source/JavaScriptCore/builtins/TypedArrayConstructor.js:57 > + if (mapFn !== undefined) { > + if (typeof mapFn !== "function") > + throw new @TypeError("TypedArray.from requires that the second argument, when provided, be a function"); You're supposed to throw when the second argument is provided and is undefined. But I don't think this code will do so because it translates missing into undefined. Please fix and add a test case. > Source/JavaScriptCore/builtins/TypedArrayConstructor.js:71 > + let accumulator = []; Why aren't we allowed to put to the result eagerly? We control the constructor function and the allocated object, so it should be fine. > Source/JavaScriptCore/builtins/TypedArrayConstructor.js:83 > + // Since for-of loop once more looks up the @@iterator property of a given iterable, > + // it could be observable if the user defines a getter for @@iterator. > + // To avoid this situation, we define a wrapper object that @@iterator just returns a given iterator. > + let wrapper = { > + [@symbolIterator]() { > + return iterator; > + } > + }; This is pretty sick :(. > Source/JavaScriptCore/builtins/TypedArrayConstructor.js:102 > + for (let i = 0; i < k; i++) { > + result[i] = accumulator[i]; > + } No braces for one line loops. > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructorInlines.h:129 > + // Here we make a slight optimization. If the iterator is a known iterator and length does not have a getter > + // then we can just not use the iterator and loop on the length instead. This comment would read more clearly if it said "We only need to do this slow thing..." instead of the other way around, since the code below it is the slow thing. You should factor the slow thing into a helper function. > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructorInlines.h:150 > + Vector<JSValue, 16> storage; This is not GC-safe. JSValues in malloc memory are not scanned. You need to use Vector<Strong<T>> or MarkedArgumentBuffer or something. Same question here about why do we need a temporary copy. > Source/JavaScriptCore/runtime/JSTypedArrayViewConstructor.cpp:86 > + while (true) { I think a for loop would read better here, and it's how we usually walk the prototype chain: for ( ; !value.isNull(); value = jsCast<JSObject*>(value)->prototype())
Comment on attachment 262922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262922&action=review >> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:37 >> + throw new @TypeError("TypedArray.from requires its this argument subclass a TypedArray constructor"); > > to subclass? > > be a subclass of? Fixed. >> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:51 >> + let mapFn = arguments.length > 1 ? arguments[1] : undefined; > > This is the same as arguments[1]. Fixed. >> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:53 >> + let thisArg; > > It seems like this function will end up reading thisArg uninitialized (TDZ). You should fix this and add a test. "let thisArg" is implicitly the same as "let thisArg = undefined". The TDZ only kicks in if you use the variable before it's initialized. Perhaps I should be explicit and put the "= undefined" in there but I believe this is correct as is. >> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:57 >> + throw new @TypeError("TypedArray.from requires that the second argument, when provided, be a function"); > > You're supposed to throw when the second argument is provided and is undefined. But I don't think this code will do so because it translates missing into undefined. Please fix and add a test case. From the spec (http://www.ecma-international.org/ecma-262/6.0/index.html#sec-%typedarray%.from): If mapfn was supplied, let f be mapfn; otherwise let f be undefined. If f is not undefined, then If IsCallable(f) is false, throw a TypeError exception. >> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:71 >> + let accumulator = []; > > Why aren't we allowed to put to the result eagerly? We control the constructor function and the allocated object, so it should be fine. I suppose we could do that, I didn't do it because I'd have to dig through the logic of how we decide where to put the backing buffer. It might not be that bad though. Is there any concern about wasting memory at the end of the buffer when the accumulation is done (i.e. should I trim my array buffer at the end)? I would imagine TypedArrays tend to be long lived but I'm not sure if the memory waste is worse than the cost of an extra memcpy. >> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:83 >> + }; > > This is pretty sick :(. D: >> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:102 >> + } > > No braces for one line loops. Fixed. >> Source/JavaScriptCore/runtime/JSTypedArrayViewConstructor.cpp:86 >> + while (true) { > > I think a for loop would read better here, and it's how we usually walk the prototype chain: > > for ( ; !value.isNull(); value = jsCast<JSObject*>(value)->prototype()) Fixed.
> >> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:53 > >> + let thisArg; > > > > It seems like this function will end up reading thisArg uninitialized (TDZ). You should fix this and add a test. > > "let thisArg" is implicitly the same as "let thisArg = undefined". The TDZ > only kicks in if you use the variable before it's initialized. Perhaps I > should be explicit and put the "= undefined" in there but I believe this is > correct as is. Oops! No, I think "let thisArg" is OK -- I just misunderstood the feature. > >> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:57 > >> + throw new @TypeError("TypedArray.from requires that the second argument, when provided, be a function"); > > > > You're supposed to throw when the second argument is provided and is undefined. But I don't think this code will do so because it translates missing into undefined. Please fix and add a test case. > > From the spec > (http://www.ecma-international.org/ecma-262/6.0/index.html#sec-%typedarray%. > from): > If mapfn was supplied, let f be mapfn; otherwise let f be undefined. > If f is not undefined, then > If IsCallable(f) is false, throw a TypeError exception. Wow. OK. > >> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:71 > >> + let accumulator = []; > > > > Why aren't we allowed to put to the result eagerly? We control the constructor function and the allocated object, so it should be fine. > > I suppose we could do that, I didn't do it because I'd have to dig through > the logic of how we decide where to put the backing buffer. It might not be > that bad though. Is there any concern about wasting memory at the end of the > buffer when the accumulation is done (i.e. should I trim my array buffer at > the end)? I would imagine TypedArrays tend to be long lived but I'm not sure > if the memory waste is worse than the cost of an extra memcpy. You explained this once before and I forgot: We need a temporary accumulator because we don't know how much the iterator will iterate, so we either need a temporary accumulator or an oversized typed array. I prefer your approach: It's better to have a typed array of the right size since the result might be large. So, all you need to do is to fix the C++ version to use a GC-safe accumulator.
Created attachment 263026 [details] Patch
Addressed Geoff's comments and added new tests.
Comment on attachment 263026 [details] Patch r=me
Committed r191059: <http://trac.webkit.org/changeset/191059>
(In reply to comment #8) > Committed r191059: <http://trac.webkit.org/changeset/191059> It broke the Apple Windows build.
Try doing a clean build. There seems to be a recurring problem that the auto generated file containing the builtin JS functions doesn't get re-built when a new file is added. I'm not sure why this happens (it only happens when a new file is added) since I added the .js file as a dependency of the generator CMake command.
(In reply to comment #10) > Try doing a clean build. There seems to be a recurring problem that the auto > generated file containing the builtin JS functions doesn't get re-built when > a new file is added. I'm not sure why this happens (it only happens when a > new file is added) since I added the .js file as a dependency of the > generator CMake command. Yeah, I just confirmed this locally. I'll log onto those bots and see if I can clean them up.
(In reply to comment #11) > (In reply to comment #10) > > Try doing a clean build. There seems to be a recurring problem that the auto > > generated file containing the builtin JS functions doesn't get re-built when > > a new file is added. I'm not sure why this happens (it only happens when a > > new file is added) since I added the .js file as a dependency of the > > generator CMake command. > > Yeah, I just confirmed this locally. I'll log onto those bots and see if I > can clean them up. In this case it is a build system bug, which should be fixed as soon as possible to avoid similar false positive failures in the future.