Summary: | Add support for Symbol.isConcatSpreadable (round 2) | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, mark.lam, msaboff, rniwa, saam | ||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Keith Miller
2016-06-14 18:07:15 PDT
Created attachment 281315 [details]
Patch
Created attachment 281318 [details]
JS-benchmarks
Created attachment 281319 [details]
JS-Bench benchmark
Comment on attachment 281315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281315&action=review > Source/JavaScriptCore/ChangeLog:9 > + do so it was necessary to move the Array.prototype.concat function Please add a comma after "so". > Source/JavaScriptCore/ChangeLog:39 > + Two new debugging tools are also added to the jsc cli. One is a Should "cli" be "CLI"? > Source/JavaScriptCore/ChangeLog:42 > + version of the print function with a private name so it can be > + used for debugging builtins. The other is dumpDataLog, which takes > + a JSValue and runs our dataLog function on it. You can already do this by using JSC_useDollarVM=1, and in builtin code do: @$vm.print("The value of this var is ", $vm.value(myVar), "\n"); Do we really need to add another version of it (especially since this is not code that is supposed to be left in the builtin)? Using $vm also has the advantage that it will be disabled by default, and if you forget to take the debugging code out, the tests will barf with an undefined $vm error. NOTE: JSC_useDollarVM is a "Restricted" option. That means, if you want to use it on release builds, you will need to modify allowRestrictedOptions() in Options.cpp to return true. It is built in by default on debug builds. Comment on attachment 281315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281315&action=review >> Source/JavaScriptCore/ChangeLog:42 >> + a JSValue and runs our dataLog function on it. > > You can already do this by using JSC_useDollarVM=1, and in builtin code do: > @$vm.print("The value of this var is ", $vm.value(myVar), "\n"); > > Do we really need to add another version of it (especially since this is not code that is supposed to be left in the builtin)? Using $vm also has the advantage that it will be disabled by default, and if you forget to take the debugging code out, the tests will barf with an undefined $vm error. > > NOTE: JSC_useDollarVM is a "Restricted" option. That means, if you want to use it on release builds, you will need to modify allowRestrictedOptions() in Options.cpp to return true. It is built in by default on debug builds. Correction: the $vm example should be: @$vm.print("The value of this var is ", @$vm.value(myVar), "\n"); I was missing the @ before $vm in the @$vm.value() case. > Source/JavaScriptCore/builtins/ArrayPrototype.js:712 > + if (@isArrayConstructor(constructor) && @Array !== constructor) > + constructor = @Array; Perf-wise (for the LLINT and baseline JIT), is it better to do the "@Array !== constructor" check, or to just skip it and just do the assignment? I suspect its better to skip the second condition. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:-4135 > - Please revert. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1017 > + return false; Please fix indentation. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1050 > + ASSERT(vm.exception()); Is this assertion correct? moveElement() can do a getProperty() on the source, and getProperty() can invoke getters. Hence, an exception can be thrown, no? > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1060 > + RELEASE_ASSERT(result->appendMemcpy(exec, vm, 0, first)); Putting this statement in a RELEASE_ASSERT suggests that it is optional when that is not the case. Can you express this as "bool appendMemcpySucceeded = ..." and "RELEASE_ASSERT(appendMemcpySucceeded);" instead? > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1076 > + // This code assumes that the neither array has set Symbol.isConcatSpreadable. If the first array Remove "the" before "neither". > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1126 > + } else if (type != ArrayWithUndecided) { > + WriteBarrier<Unknown>* buffer = result->butterfly()->contiguous().data(); > + memcpy(buffer, firstButterfly->contiguous().data(), sizeof(JSValue) * firstArraySize); > + memcpy(buffer + firstArraySize, secondButterfly->contiguous().data(), sizeof(JSValue) * secondArraySize); > + } I presume (type == ArrayWithUndecided) means that the length of both are 0. Can you make that explicit here with an else case that asserts the following? ASSERT(!firstArraySize && !secondArraySize); > Source/JavaScriptCore/runtime/JSArray.cpp:411 > + ASSERT(copyType == ArrayWithUndecided); > + return true; Please add an assertion here that makes it clear that ArrayWithUndecided means 0 length: ASSERT(!otherArray->length()); > Source/JavaScriptCore/runtime/JSArray.cpp:427 > + if (length() < newLength) { > + throwOutOfMemoryError(exec); > + return false; > + } Shouldn't ensureLength() already guarantee that (length() >= newLength)? You should move this throwOutOfMemoryError() into the fail case for ensureLength() above. > Source/JavaScriptCore/runtime/JSArrayInlines.h:29 > +IndexingType JSArray::memCopyWithIndexingType(IndexingType other) This name "memCopyWithIndexingType" is a bit misleading because it implies that it will do a memcpy. How about naming it something like "resultIndexingTypeForMemCopy" or "computeResultIndexingTypeForMemCopy"instead? Created attachment 281404 [details]
Patch
Created attachment 281407 [details]
Patch
Attachment 281407 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1064: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 60 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 281410 [details]
Patch
Comment on attachment 281410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281410&action=review r=me with one fix (assuming EWS bots will be happy). > Source/JavaScriptCore/builtins/ArrayPrototype.js:713 > + if (@isObject(constructor)) { I think you should make this an "else if" since @isObject() should always fail for @undefined. Created attachment 281415 [details]
Patch for landing
Comment on attachment 281415 [details] Patch for landing Attachment 281415 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1509985 New failing tests: js/array-species-different-globalobjects.html Created attachment 281419 [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 281415 [details] Patch for landing Attachment 281415 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1509969 New failing tests: js/array-species-different-globalobjects.html Created attachment 281420 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Comment on attachment 281415 [details] Patch for landing Attachment 281415 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1509990 New failing tests: js/array-species-different-globalobjects.html Created attachment 281421 [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 281415 [details] Patch for landing Attachment 281415 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1509987 New failing tests: js/array-species-different-globalobjects.html js/kde/Array.html Created attachment 281422 [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 281426 [details]
Patch for landing
> I presume (type == ArrayWithUndecided) means that the length of both are 0.
> Can you make that explicit here with an else case that asserts the following?
>
> ASSERT(!firstArraySize && !secondArraySize);
>
> > Source/JavaScriptCore/runtime/JSArray.cpp:411
> > + ASSERT(copyType == ArrayWithUndecided);
> > + return true;
>
> Please add an assertion here that makes it clear that ArrayWithUndecided
> means 0 length:
> ASSERT(!otherArray->length());
>
These assertions are not right. We use ArrayWithUndecided for arrays with non-zero length but no elements in them. For instance, Array(5).
(In reply to comment #21) > > Please add an assertion here that makes it clear that ArrayWithUndecided > > means 0 length: > > ASSERT(!otherArray->length()); > > > > These assertions are not right. We use ArrayWithUndecided for arrays with > non-zero length but no elements in them. For instance, Array(5). You're right. Sorry for my mistake. Please remove those asserts. Comment on attachment 281426 [details] Patch for landing Clearing flags on attachment: 281426 Committed r202125: <http://trac.webkit.org/changeset/202125> All reviewed patches have been landed. Closing bug. I am working on confirming but this looks like a 1% PLT regression on iOS :( This also appears to be 1% JSBench regression on iPhone 6 Plus. (In reply to comment #25) > I am working on confirming but this looks like a 1% PLT regression on iOS :( Confirmed slight regression on iOS. |