Implement the %TypedArray%.prototype.includes ( searchElement [ , fromIndex ] ) following the ECMAScript 2016 draft version available in https://tc39.github.io/ecma262/2016/#sec-%typedarray%.prototype.includes
Created attachment 282639 [details] RFC Patch
Attachment 282639 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 282639 [details] RFC Patch This Patch contains the implementation of JavaScript version and also the C++ version. I've made a benchmark with this code: let a = new Float64Array(100); a.fill(1); for (let i = 0; i < 100000000; i++) { a.includes(0); } The idea is reproduce the worst case, since it is going to compare with all array elements. These are the configurations of runs: ----------C++ Implementation---------- Caios-MacBook-Pro:js-tests caiolima$ JSC_useJIT=false time run-jsc includes-benchmark.js Running 1 time(s): DYLD_FRAMEWORK_PATH=/Users/caiolima/open_projects/WebKit/WebKitBuild/Release /Users/caiolima/open_projects/WebKit/WebKitBuild/Release/jsc includes-benchmark.js 16.36 real 16.04 user 0.16 sys Caios-MacBook-Pro:js-tests caiolima$ JSC_useJIT=true time run-jsc includes-benchmark.js Running 1 time(s): DYLD_FRAMEWORK_PATH=/Users/caiolima/open_projects/WebKit/WebKitBuild/Release /Users/caiolima/open_projects/WebKit/WebKitBuild/Release/jsc includes-benchmark.js 13.71 real 13.40 user 0.16 sys ----------JS Wraper Implementation---------- Caios-MacBook-Pro:js-tests caiolima$ JSC_useJIT=false time run-jsc includes-benchmark.js Running 1 time(s): DYLD_FRAMEWORK_PATH=/Users/caiolima/open_projects/WebKit/WebKitBuild/Release /Users/caiolima/open_projects/WebKit/WebKitBuild/Release/jsc includes-benchmark.js 345.25 real 343.33 user 1.24 sys Caios-MacBook-Pro:js-tests caiolima$ Caios-MacBook-Pro:js-tests caiolima$ JSC_useJIT=true time run-jsc includes-benchmark.js Running 1 time(s): DYLD_FRAMEWORK_PATH=/Users/caiolima/open_projects/WebKit/WebKitBuild/Release /Users/caiolima/open_projects/WebKit/WebKitBuild/Release/jsc includes-benchmark.js 12.54 real 12.22 user 0.23 sys What I think is the C++ implementation has better LLInt - JIT balance, since it performs well with and without JIT. However, When the javascript wrapper is JITed,it performs better than the C++ version JITed. What do you guys think about it. Also, I am open to new Benchmark ideas.
Comment on attachment 282639 [details] RFC Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282639&action=review An implementation of TypedArray's includes, that's awesome! Some comments bellow: > Source/JavaScriptCore/ChangeLog:10 > + %TypedArray%.prototype.includes following spec 22.2.3.14 > + https://tc39.github.io/ecma262/2016/#sec-%typedarray%.prototype.includes Be careful to use spaces for indentation. We avoid mixing tabs and spaces, the rule is to always use spaces. > Source/JavaScriptCore/ChangeLog:19 > + Don't forget to add test coverage too! You should have tests in LayoutTests/js that try various use cases and verify the correctness. There should also be a test getting the property descriptor and checking its properties (name, enumerable, etc). > Source/JavaScriptCore/builtins/TypedArrayPrototype.js:355 > + fromIndex = arguments[1] | 0; This looks suspicious to me. The specs says: Let n be ? ToInteger(fromIndex). (If fromIndex is undefined, this step produces the value 0.) ToInteger() produces integer for values > INT_MAX. x | 0 is a ToInt32() on x. The bounds are [INT_MIN, INT_MAX] Looking at Array.prototype, I suspect we have a bug there. > Source/JavaScriptCore/builtins/TypedArrayPrototype.js:370 > + if (searchElement === currentElement || (searchElement !== searchElement && currentElement !== currentElement)) The loop would benefit from being specialized on searchElement !== searchElement. if (searchElement === searchElement) { ... Loop searching for searchElement } else { ... Loop searching for NaN } > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222 > + if (array[index] == target) Isn't that missing the NaN case? > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:223 > + return JSValue::encode(JSValue(JSValue::JSTrue)); You can also use jsBoolean(true/false) to produce a boolean JSValue.
Created attachment 282681 [details] Patch
(In reply to comment #4) > Comment on attachment 282639 [details] > RFC Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282639&action=review > > An implementation of TypedArray's includes, that's awesome! > > Some comments bellow: > > > Source/JavaScriptCore/ChangeLog:10 > > + %TypedArray%.prototype.includes following spec 22.2.3.14 > > + https://tc39.github.io/ecma262/2016/#sec-%typedarray%.prototype.includes > > Be careful to use spaces for indentation. > We avoid mixing tabs and spaces, the rule is to always use spaces. Sorry, my vim is not configured correctly. Thanks for point it =). > > Source/JavaScriptCore/ChangeLog:19 > > + > > Don't forget to add test coverage too! > > You should have tests in LayoutTests/js that try various use cases and > verify the correctness. > There should also be a test getting the property descriptor and checking its > properties (name, enumerable, etc). I added coverage, but I didn't consider cases about property descriptor. I am going to update it soon. > > Source/JavaScriptCore/builtins/TypedArrayPrototype.js:355 > > + fromIndex = arguments[1] | 0; > > This looks suspicious to me. > > The specs says: > Let n be ? ToInteger(fromIndex). (If fromIndex is undefined, this step > produces the value 0.) > > ToInteger() produces integer for values > INT_MAX. > > x | 0 is a ToInt32() on x. The bounds are [INT_MIN, INT_MAX] > > Looking at Array.prototype, I suspect we have a bug there. I agree with you. I double checked the spec and confirm it. However, I am not sure if it is possible create an Array with length > INT_MAX. I need to check it. > > Source/JavaScriptCore/builtins/TypedArrayPrototype.js:370 > > + if (searchElement === currentElement || (searchElement !== searchElement && currentElement !== currentElement)) > > The loop would benefit from being specialized on searchElement !== > searchElement. > > if (searchElement === searchElement) { > ... Loop searching for searchElement > } else { > ... Loop searching for NaN > } Nice! However I decided pick the C++ version in the new Patch. > > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222 > > + if (array[index] == target) > > Isn't that missing the NaN case? Now I am considering it. However I have a doubt with suspicious cases: 1. If the searchElement is an Object or a String? In IntXArray, they are converted to 0, so "new UInt8Array([0, 1, 2]).includes("abc"); // is true". It is not just the case of %TypedArray%.prototype.includes, but also %TypedArray%.prototype.indexOf. I didn't find any information about it in the Spec. I checked v8 implementation and they return false to these cases. IMHO, I think it is the best result, since "new UInt8Array([0, 1, 2]).includes("abc");" returning "true" is a potential unpredictable bug source. > > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:223 > > + return JSValue::encode(JSValue(JSValue::JSTrue)); > > You can also use jsBoolean(true/false) to produce a boolean JSValue. Thank you for this Tip!
Comment on attachment 282681 [details] Patch Attachment 282681 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1622898 New failing tests: js/regress/typed-array-includes.html
Created attachment 282685 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 282681 [details] Patch Attachment 282681 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1622899 New failing tests: js/regress/typed-array-includes.html
Created attachment 282686 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 282681 [details] Patch Attachment 282681 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1622900 New failing tests: js/regress/typed-array-includes.html
Created attachment 282688 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 282681 [details] Patch Attachment 282681 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1622901 New failing tests: js/regress/typed-array-includes.html
Created attachment 282689 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Created attachment 282691 [details] Patch
> > > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222 > > > + if (array[index] == target) > > > > Isn't that missing the NaN case? > > Now I am considering it. However I have a doubt with suspicious cases: > > 1. If the searchElement is an Object or a String? In IntXArray, they are > converted to 0, so "new UInt8Array([0, 1, 2]).includes("abc"); // is true". > It is not just the case of %TypedArray%.prototype.includes, but also > %TypedArray%.prototype.indexOf. I didn't find any information about it in > the Spec. I checked v8 implementation and they return false to these cases. > IMHO, I think it is the best result, since "new UInt8Array([0, 1, > 2]).includes("abc");" returning "true" is a potential unpredictable bug > source. You are right, new UInt8Array([0, 1, 2]).includes("abc") and new UInt8Array([0, 1, 2]).indexOf("abc") should return false. Converting the string to a number makes no sense. The spec does not say to convert the input to a number: -https://tc39.github.io/ecma262/#sec-%typedarray%.prototype.indexof -https://tc39.github.io/ecma262/#sec-array.prototype.indexof You have discovered a really stupid bug in the existing indexOf!
Created attachment 282739 [details] Patch
Created attachment 282752 [details] Patch
Comment on attachment 282752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282752&action=review Second round of review: > Source/JavaScriptCore/ChangeLog:6 > + Reviewed by Benjamin Poulain. You don't need to modify this field. The tool that lands the page (webkit-patch) replaces "NOBODY (OOPS!)" by the name of the reviewer found on bugzilla. > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:211 > + TypedArrayType arrayType = exec->thisValue().getObject()->classInfo()->typedArrayStorageType; This is less efficient than it should. You are getting the array type at runtime. This condition can be fully determined at compile time from the template argument ViewClass. > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:216 > + if (!valueToFind.isNumber() && !valueToFind.isDouble()) Isn't "isNumber()" a superset of "isDouble()"? valueToFind.isNumber() false implies valueToFind.isDouble() false. > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222 > + typename ViewClass::ElementType target = ViewClass::toAdaptorNativeFromValue(exec, valueToFind); I don't think that works for includes. For example, Uint8ClampedAdaptor will clamp positive values to 255, that's not what we need here. IntegralTypedArrayAdaptor does ToInt32 on the input, that's not what we need either. Your test should have extensive coverage of those crazy cases. ---- What you probably need is a new Adaptor function that returns a value in the right type or an error. For example, if you pass a double to an integer type array, it returns the integer if it fits in the array type or fail. E.g.: -The double 256.0 works for int16, fails for int8 -The double 254.1 fails for all integers. > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:226 > + if (std::isnan(target)) { This can be eliminated at compile time with the test described above for ViewClass. > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:236 > + if (JSValue::strictEqual(exec, currentValue, valueToFind)) I think it is not a good idea to use strictEqual() for performance. The type of the Array is known, and the type of the valueToFind can be converted to that type. > LayoutTests/ChangeLog:13 > + * js/regress/script-tests/typed-array-includes.js: Added. Any reason this is in js/regress and not in js/ > LayoutTests/js/regress/script-tests/typed-array-includes.js:1 > +function assert(a) { You can use shouldBeTrue(), available in js-test-pre.js. > LayoutTests/js/regress/script-tests/typed-array-includes.js:22 > + assert(!tArray.includes("abc")); It is worth testing all the types: undefined, null, empty string, regular object. > LayoutTests/js/regress/script-tests/typed-array-includes.js:96 > + Some things worth checking too: Object.getPrototypeOf(TypedArray).prototype.name === "includes" Object.getPrototypeOf(TypedArray).prototype.length === 1
Comment on attachment 282752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282752&action=review >> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:211 >> + TypedArrayType arrayType = exec->thisValue().getObject()->classInfo()->typedArrayStorageType; > > This is less efficient than it should. > > You are getting the array type at runtime. > This condition can be fully determined at compile time from the template argument ViewClass. Yeah the TypedArrayType can be found with ViewClass::TypedArrayStorageType
Comment on attachment 282752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282752&action=review >> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:216 >> + if (!valueToFind.isNumber() && !valueToFind.isDouble()) > > Isn't "isNumber()" a superset of "isDouble()"? > > valueToFind.isNumber() false implies valueToFind.isDouble() false. You are right. I was considering isDouble because of NaN case, bit Ironically, NaN isNumber is true. >> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222 >> + typename ViewClass::ElementType target = ViewClass::toAdaptorNativeFromValue(exec, valueToFind); > > I don't think that works for includes. > > For example, Uint8ClampedAdaptor will clamp positive values to 255, that's not what we need here. > IntegralTypedArrayAdaptor does ToInt32 on the input, that's not what we need either. > > Your test should have extensive coverage of those crazy cases. > > ---- > > What you probably need is a new Adaptor function that returns a value in the right type or an error. > > For example, if you pass a double to an integer type array, it returns the integer if it fits in the array type or fail. > E.g.: > -The double 256.0 works for int16, fails for int8 > -The double 254.1 fails for all integers. Nice point. I am working in that now.
Hey Benjamin and Keith, Thank you for you review. I learned a lot from your comments! I am going to update my patch now following your directions, since all of them makes sense to me.
Created attachment 283021 [details] Patch
Comment on attachment 283021 [details] Patch Attachment 283021 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1642077 New failing tests: js/typed-array-includes.html
Created attachment 283025 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 283021 [details] Patch Attachment 283021 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1642082 New failing tests: js/typed-array-includes.html
Created attachment 283026 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 283021 [details] Patch Attachment 283021 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1642092 New failing tests: js/typed-array-includes.html
Created attachment 283029 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Comment on attachment 283021 [details] Patch Attachment 283021 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1642085 New failing tests: js/typed-array-includes.html
Created attachment 283030 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 283112 [details] Patch
You seem to have trouble getting test expectations. What's your workflow to get them?
(In reply to comment #33) > You seem to have trouble getting test expectations. > > What's your workflow to get them? Yes, it was a painful process. I built all the webkit and un the test in Safari to get the output. After that I changed the file with the current output but I missed the newline. I uploaded the Patch to check if it was ok, because When I tried to run "run-webkit-test" in the morning, the webkit started compile from scratch agin...Now it is fixed (at least localhost) and I am going to check if the win build is not going to fail. BTW, I faced a strange behavior...Even with new Float(32|64)Array([NaN]), when searchValue=NaN and I perform a loop through elements, array[i] = NaN and target = NaN but array[i] == target returns false...Can you explain it?
Created attachment 283116 [details] Patch
(In reply to comment #34) > (In reply to comment #33) > > You seem to have trouble getting test expectations. > > > > What's your workflow to get them? > > Yes, it was a painful process. I built all the webkit and un the test in > Safari to get the output. After that I changed the file with the current > output but I missed the newline. I uploaded the Patch to check if it was ok, > because When I tried to run "run-webkit-test" in the morning, the webkit > started compile from scratch agin...Now it is fixed (at least localhost) and > I am going to check if the win build is not going to fail. Ok. Here are two ways you can update the expectations: If you build the full WebKit (sometimes necessary, not always), you can run the test you want and use --reset-results. For example, in your case you can do: ./Tools/Scripts/run-webkit-test js/typed-array-includes.html --reset-results This will run the test and replace the -expected.txt file of that test. (Note that the command takes the path to the test without "LayoutTest" in front, which is a bit weird) If you do not need to run any of the WebKit test, you can sometime get away with just building JavaScript. What you do then is run the script with standalone-pre.js and standalone-post.js. For example for your new test I would try: DYLD_FRAMEWORK_PATH=./WebKitBuild/Release/ ./WebKitBuild/Release/jsc LayoutTest/resources/standalone-pre.js LayoutTests/js/script-tests/typed-array-includes.js LayoutTest/resources/standalone-post.js > LayoutTests/js/typed-array-includes-expected.txt A bit ugly, but it gets the job done. > BTW, I faced a strange behavior...Even with new Float(32|64)Array([NaN]), > when searchValue=NaN and I perform a loop through elements, array[i] = NaN > and target = NaN but array[i] == target returns false...Can you explain it? I am not certain to understand the question but if you are wondering about NaN equality...: If you do NaN === NaN, that's false. A NaN is not equal to itself (or any other number). Even in C, if you have double value = NaN; (value == value) -> This is false. Since NaN is not a number, comparing it to anything is false. In practice, CPUs have two kinds of comparisons: ordered and unordered. -Ordered comparison is true if neither operands are NaN. -An unordered comparison is true if either operands are NaN. If you look at the "DoubleCondition" enum in any of the MacroAssembler, you'll get an idea of what comparison operator are available. --- If I misunderstood your question: can you give an complete example?
Thank you for these information! They are extremely valuable to my workflow. It is the reason that I didn't remove the if-else clause checking the NaN case. Now everything is green, finally. After this patch being r+ and accepted, I am going to resuse the work done here to fix TypedArray.indexOf implementation.
Comment on attachment 283116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283116&action=review Third review round. This is getting in really good shape. But I think I found a couple of bugs: > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:209 > + if (!valueToFind.isNumber()) > + return JSValue::encode(jsBoolean(false)); I believe having this here is a bug. It is not spec compliant. If you check https://tc39.github.io/ecma262/2016/#sec-array.prototype.includes, step 4. is calling ToInteger() on the index. Here, if the argument zero is not a number, we exit *before* calling ToInteger. Why is that important? The second argument can be an object with "valueOf". This optimization will skip the call to valueOf. You should add a test verifying that passing a string as first argument and an object as second argument calls valueOf() exactly once on the object. > Source/JavaScriptCore/runtime/ToNativeFromValue.h:58 > + return Adaptor::toNativeFromDouble(value.toNumber(exec), result); I think this is a bug too. You are calling toNumber() on the value. If valueToFind is an Object, that will call valueOf() on it. If it's null or undefined, it will be converted to number too. You just need to return false here. You need a test covering this case. > Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:104 > + Type ans = static_cast<Type>(value); Rename ans->integer? > Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:163 > + static Type toNativeFromUint32(uint32_t value, Type& result) Is this needed? Where is it called? > Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:177 > + if (value < minValue || value > maxValue) > + return false; This is not really enough because of double->float conversion. A double can be bigger than minValue and less than maxValue yet not convert cleanly to a float. This happens if the double has a higher precision than the float. What we need is (static_cast<double>(static_cast<Type>(value)) == value). This ensure that the conversion to float and back to double does not lose precision. This is valid because you have a NaN check above so we don't have to deal with non-finite numbers. It's probably possible to write a test covering this. > Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:278 > + static bool toNativeFromUint32(uint32_t value, Type& result) Is this needed? > Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:290 > + int32_t ans = static_cast<int32_t>(value); Rename ans->integer. I believe you could even type it uint8_t and skip the "if (ans > maxValue || ans < minValue)" below.
Comment on attachment 283116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283116&action=review Good points in comment. Thank you for the contributions, mainly because I didn't pay attention to float -> double case! >> Source/JavaScriptCore/runtime/ToNativeFromValue.h:58 >> + return Adaptor::toNativeFromDouble(value.toNumber(exec), result); > > I think this is a bug too. > > You are calling toNumber() on the value. > If valueToFind is an Object, that will call valueOf() on it. If it's null or undefined, it will be converted to number too. > > You just need to return false here. > > You need a test covering this case. Actually, we have test covering this case. Check LayoutTests/js/script-tests/typed-array-includes.js:17-23. When this function is called from "genericTypedArrayViewProtoFuncIncludes", we are sure that value.isNumber() is true. However, maintaining this last case I think it keeps the function more reusable.
Created attachment 283270 [details] Patch
Comment on attachment 283270 [details] Patch Attachment 283270 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1654942 New failing tests: js/typed-array-includes.html
Created attachment 283271 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 283270 [details] Patch Attachment 283270 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1654945 New failing tests: js/typed-array-includes.html
Created attachment 283272 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 283270 [details] Patch Attachment 283270 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1654949 New failing tests: js/typed-array-includes.html
Created attachment 283273 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 283270 [details] Patch Attachment 283270 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1654948 New failing tests: js/typed-array-includes.html
Created attachment 283274 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.5
Created attachment 283277 [details] Patch
Comment on attachment 283277 [details] Patch Fantastic work. Great job on the test coverage. Let's land!
(In reply to comment #50) > Comment on attachment 283277 [details] > Patch > > Fantastic work. Great job on the test coverage. > Let's land! Thank you very much! I really liked your review because I learned a lot of new things and also we could catch a lot of "hidden" cases. It was a good example that 2 heads think better than one =). Tomorrow morning I am going to start work in the %TypedArray%.prototype.indexOf bug and this patch is going to be helpful there.
Comment on attachment 283277 [details] Patch Clearing flags on attachment: 283277 Committed r203037: <http://trac.webkit.org/changeset/203037>
All reviewed patches have been landed. Closing bug.
https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/3404/ https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/7277 It seems that this patch causes JSC test failures. Could you take a look?
(In reply to comment #54) > https://build.webkit.org/builders/ > Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/3404/ > https://build.webkit.org/builders/ > Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/7277 > > It seems that this patch causes JSC test failures. Could you take a look? Hey Yusuke, is it possible get the output of this LayoutTests run?
(In reply to comment #55) > (In reply to comment #54) > > https://build.webkit.org/builders/ > > Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/3404/ > > https://build.webkit.org/builders/ > > Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/7277 > > > > It seems that this patch causes JSC test failures. Could you take a look? > > Hey Yusuke, is it possible get the output of this LayoutTests run? The results of the layout-tests can be seen in https://build.webkit.org/results/ :)
(In reply to comment #56) > (In reply to comment #55) > > (In reply to comment #54) > > > https://build.webkit.org/builders/ > > > Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/3404/ > > > https://build.webkit.org/builders/ > > > Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/7277 > > > > > > It seems that this patch causes JSC test failures. Could you take a look? > > > > Hey Yusuke, is it possible get the output of this LayoutTests run? > > The results of the layout-tests can be seen in > https://build.webkit.org/results/ :) Well, the problem is weird. When I run "run-javascriptcore-tests", the output is different from "run-webkit-tests". Here is the diff: --- /Users/caiolima/open_projects/WebKit/WebKitBuild/Debug/layout-test-results/js/typed-array-includes-expected.txt +++ /Users/caiolima/open_projects/WebKit/WebKitBuild/Debug/layout-test-results/js/typed-array-includes-actual.txt @@ -1,3 +1,8 @@ +JSRegress/typed-array-includes + +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". + + PASS tArray.includes(2) is true PASS !tArray.includes(4) is true PASS !tArray.includes(3, 3) is true @@ -251,6 +256,7 @@ PASS funcCallCount.callCount === 4 is true PASS funcCallCount.callCount === 5 is true PASS funcCallCount.callCount === 6 is true +PASS no exception thrown PASS successfullyParsed is true TEST COMPLETE I can move the test from LayoutTests to Source/JavaScriptCore/tests and avoid it breaks the "LayoutTests". However, I would like to know why this difference happens. Do you have any tips?
Re-opened since this is blocked by bug 159614
Created attachment 283383 [details] Patch
Comment on attachment 283383 [details] Patch I'll wait for the bots before cq+.
(In reply to comment #60) > Comment on attachment 283383 [details] > Patch > > I'll wait for the bots before cq+. It is green now.
(In reply to comment #60) > Comment on attachment 283383 [details] > Patch > > I'll wait for the bots before cq+. Sorry, I was on the road.
Comment on attachment 283383 [details] Patch Clearing flags on attachment: 283383 Committed r203107: <http://trac.webkit.org/changeset/203107>
*** Bug 156982 has been marked as a duplicate of this bug. ***