We should support this.
Created attachment 274305 [details] Patch
Created attachment 274308 [details] Patch
Attachment 274308 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:295: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 274310 [details] Benchmark results
Results with 10 reruns on Octane/Kraken (I assume json-parse-financial is unrelated): TOT Conf#2 Octane: encrypt 1.48749+-0.04154 1.47919+-0.06296 decrypt 26.80845+-0.61037 26.76883+-0.73212 deltablue x2 1.28161+-0.02215 ? 1.28218+-0.05821 ? earley 2.69246+-0.01478 2.68168+-0.02034 boyer 44.10906+-2.09822 43.41107+-0.24304 might be 1.0161x faster navier-stokes x2 45.87185+-0.69001 ? 46.03705+-0.98245 ? raytrace x2 8.37932+-0.07353 8.35955+-0.03899 richards x2 0.76783+-0.02436 0.76344+-0.01918 splay x2 3.26142+-0.05023 3.25891+-0.07848 regexp x2 190.28251+-2.03038 ? 190.34545+-14.83350 ? pdfjs x2 346.79246+-8.67128 ? 349.91664+-5.86553 ? mandreel x2 362.50135+-3.07694 361.60019+-2.45072 gbemu x2 223.84159+-0.79068 ? 224.27118+-1.42336 ? closure 5.46698+-0.02357 ? 5.47181+-0.03893 ? jquery 70.71302+-1.24316 ? 71.11153+-0.94862 ? box2d x2 86.37910+-0.75527 86.27665+-0.85182 zlib x2 2035.71659+-23.94612 2021.69727+-34.57433 typescript x2 3794.22253+-115.38443 3786.35999+-27.57660 <geometric> 44.84098+-0.22196 44.78911+-0.41718 might be 1.0012x faster TOT Conf#2 Kraken: ai-astar 838.869+-26.346 ? 839.639+-34.228 ? audio-beat-detection 448.569+-2.983 ? 452.673+-1.979 ? audio-dft 836.956+-7.875 ? 842.762+-10.828 ? audio-fft 352.547+-1.394 ? 358.491+-6.047 ? might be 1.0169x slower audio-oscillator 446.141+-2.810 445.885+-2.802 imaging-darkroom 572.133+-13.260 567.848+-8.067 imaging-desaturate 424.027+-13.189 ? 430.323+-37.076 ? might be 1.0148x slower imaging-gaussian-blur 591.073+-15.968 587.380+-32.795 json-parse-financial 344.676+-4.265 ! 353.683+-4.299 ! definitely 1.0261x slower json-stringify-tinderbox 212.391+-3.579 ? 217.195+-8.337 ? might be 1.0226x slower stanford-crypto-aes 386.629+-7.136 384.526+-5.071 stanford-crypto-ccm 323.469+-9.658 302.488+-11.661 might be 1.0694x faster stanford-crypto-pbkdf2 922.743+-7.144 911.586+-14.156 might be 1.0122x faster stanford-crypto-sha256-iterative 370.241+-4.056 ^ 357.123+-5.823 ^ definitely 1.0367x faster <arithmetic> 505.033+-2.092 503.686+-2.581 might be 1.0027x faster TOT Conf#2 Geomean of preferred means: <scaled-result> 150.4864+-0.6535 150.1984+-0.9704 might be 1.0019x faster
Comment on attachment 274308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274308&action=review > Source/JavaScriptCore/ChangeLog:16 > + 4) ToObject: Attempts to convert the first child into an object. It would help the summary if you said here what happens when ToObject fails in its attempt: returns null? OSR exits? > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1054 > + if (!(child.m_type & ~SpecArray)) { > + setConstant(node, jsBoolean(true)); > + constantWasSet = true; > + break; > + } > + Nice. You could also return false if !(m_child.m_type & SpecObject). > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2124 > + ASSERT(argumentCountIncludingThis == 2); So, you're asserting because this is only used from builtins? I'm not sure I like that. :-( I think that it's nice to have reduce the strength of the contract between DFG and builtins, so that if you call a builtin with more args, you don't crash. > Source/JavaScriptCore/dfg/DFGClobberize.h:426 > - > + Revert
Comment on attachment 274308 [details] Patch Attachment 274308 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/995461 New failing tests: js/dom/array-prototype-properties.html
Created attachment 274315 [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 274308 [details] Patch Attachment 274308 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/995467 New failing tests: js/dom/array-prototype-properties.html
Created attachment 274317 [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 274308 [details] Patch Attachment 274308 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/995466 New failing tests: js/dom/array-prototype-properties.html
Created attachment 274318 [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.10.5
Comment on attachment 274308 [details] Patch Attachment 274308 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/995494 New failing tests: inspector/codemirror/prettyprinting-javascript.html jquery/manipulation.html jquery/traversing.html js/kde/Array.html inspector/console/command-line-api.html js/dom/array-prototype-properties.html
Created attachment 274321 [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
Comment on attachment 274308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274308&action=review >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2124 >> + ASSERT(argumentCountIncludingThis == 2); > > So, you're asserting because this is only used from builtins? > > I'm not sure I like that. :-( I think that it's nice to have reduce the strength of the contract between DFG and builtins, so that if you call a builtin with more args, you don't crash. We also have the same assertion in the native function implementation as well. I would agree that such an assertion is not ideal were that the not the case. As it stands right now it's just a continuation of the same sanity check. >> Source/JavaScriptCore/dfg/DFGClobberize.h:426 >> + > > Revert Fixed.
Comment on attachment 274308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274308&action=review >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1054 >> + > > Nice. You could also return false if !(m_child.m_type & SpecObject). Good call. I've added that case.
Created attachment 274343 [details] Patch
Attachment 274343 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:295: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 274348 [details] Patch
Attachment 274348 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:295: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 274343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274343&action=review > Source/JavaScriptCore/builtins/ArrayPrototype.js:655 > + var resultIsArray = @isActualArray(result); Maybe "isUnproxiedArray"? Or "isPlainArray"? > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:974 > + setConstant(node, jsBoolean(true)); Do you want to fall through here? > Source/JavaScriptCore/dfg/DFGNodeType.h:295 > + /* I'd like to call this IsArray but then we get namespace problems with the indexing type name. Also, it is marked must generate because it can throw. */ \ nit: not sure the namespace conflicts part of the comment is helpful.
Created attachment 274351 [details] Patch
Attachment 274351 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:295: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 274351 [details] Patch Attachment 274351 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/996764 New failing tests: js/dom/array-prototype-properties.html
Created attachment 274355 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 274351 [details] Patch Attachment 274351 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/996769 New failing tests: js/dom/array-prototype-properties.html
Created attachment 274356 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 274351 [details] Patch Attachment 274351 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/996813 New failing tests: js/dom/array-prototype-properties.html
Created attachment 274359 [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 274351 [details] Patch Attachment 274351 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/996773 New failing tests: js/dom/array-prototype-properties.html
Created attachment 274360 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Tests just need to be rebaselined I'll do that before landing.
Comment on attachment 274351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274351&action=review I have some comments. I haven't looked over everything, but I think there is enough here to address. > Source/JavaScriptCore/builtins/ArrayPrototype.js:655 > + var resultIsArray = @isActualArray(result); I still like "isUnproxiedArray" for the name of this function. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1836 > + destination.merge(SpecObject); Why merge here and not set the type to be SpecObject? > Source/JavaScriptCore/dfg/DFGNodeType.h:295 > + /* I'd like to call this IsArray but then we get namespace problems with the indexing type name. Also, it is marked must generate because it can throw. */ \ Still not a fan of the namespace comment. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3302 > + JSValueOperand value(this, node->child1()); > + GPRFlushedCallResult result(this); > + > + JSValueRegs valueRegs = value.jsValueRegs(); > + GPRReg resultGPR = result.gpr(); > + > + JITCompiler::Jump isNotCell = m_jit.branchIfNotCell(valueRegs); > + > + JITCompiler::Jump notArray = m_jit.branch8(JITCompiler::NotEqual, > + JITCompiler::Address(valueRegs.payloadGPR(), JSCell::typeInfoTypeOffset()), > + TrustedImm32(ArrayType)); > + moveTrueTo(resultGPR); > + JITCompiler::Jump isArrayObject = m_jit.jump(); > + > + notArray.link(&m_jit); > + flushRegisters(); > + callOperation(operationIsArrayObject, resultGPR, valueRegs); > + blessBoolean(resultGPR); > + JITCompiler::Jump isProxyObject = m_jit.jump(); > + > + isNotCell.link(&m_jit); > + moveFalseTo(resultGPR); > + > + isProxyObject.link(&m_jit); > + isArrayObject.link(&m_jit); > + blessedBooleanResult(resultGPR, node); I'm not sure your use of flushRegisters is correct here. I think it should be a silentFlush because there are paths that don't need flushing. Also, the names of your jumps are confusing to me. I think it's be easier to read just if you had a done JumpList. It might also be worth adding a FIXME to inline all of "isArray" here and only jump to the slow path when we've encountered a revoked proxy so we can throw an exception. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3316 > +void SpeculativeJIT::compileIsArrayConstructor(Node* node) > +{ > + JSValueOperand value(this, node->child1()); > + GPRFlushedCallResult result(this); > + > + JSValueRegs valueRegs = value.jsValueRegs(); > + GPRReg resultGPR = result.gpr(); > + > + flushRegisters(); > + callOperation(operationIsArrayConstructor, resultGPR, valueRegs); > + unblessedBooleanResult(resultGPR, node); > +} I just looked at the code for "arrayConstructorPrivateFuncIsArrayConstructor" and I feel like we could do a lot better here. That function just does a JSDyanmicCast on ArrayConstructor, but I don't think we have any inheritors of that class. Why not just do a class info equality check here instead? > Source/JavaScriptCore/runtime/ArrayConstructor.h:100 > + { > + if (!argumentValue.isObject()) > + return false; > + > + return jsCast<JSObject*>(argumentValue)->classInfo() == ArrayConstructor::info(); > + } Style: indentation. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1029 > + // We can Unfinished comment. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1036 > + if (vm.exception()) > + return false; can this throw? > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1080 > + auto buffer = result->butterfly()->contiguousDouble().data(); I know we use auto elsewhere for similar operations, but I think it's easier to verify the memcpy is correct if you wrote out "double* buffer" here and below.
Comment on attachment 274351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274351&action=review >> Source/JavaScriptCore/builtins/ArrayPrototype.js:655 >> + var resultIsArray = @isActualArray(result); > > I still like "isUnproxiedArray" for the name of this function. I like IsActualArray since I plan, in the near future, to make runtime arrays not be JSArrays anymore so IsActualArray will just refer to ArrayPrototype and JSArrays. As a note, I'd like to make runtime arrays not subclasses of JSArrays since they are different enough in how they are laid out it becomes inconvenient to deal with. I would consider renaming it to isJSArray, however, what are your thoughts that name? >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1836 >> + destination.merge(SpecObject); > > Why merge here and not set the type to be SpecObject? That's a good point. I was copying ToThis bock that was probably a mistake since ToThis is poorly implemented. I'll change. >> Source/JavaScriptCore/dfg/DFGNodeType.h:295 >> + /* I'd like to call this IsArray but then we get namespace problems with the indexing type name. Also, it is marked must generate because it can throw. */ \ > > Still not a fan of the namespace comment. I think it's useful as someone might want to change the name to IsArray in the future. It would be annoying to spend the time trying to change the name only to find out it doesn't work. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3302 >> + blessedBooleanResult(resultGPR, node); > > I'm not sure your use of flushRegisters is correct here. I think it should be a silentFlush because > there are paths that don't need flushing. Also, the names of your jumps are confusing to me. > I think it's be easier to read just if you had a done JumpList. It might also be worth adding a FIXME > to inline all of "isArray" here and only jump to the slow path when we've encountered a revoked proxy > so we can throw an exception. I think you are correct about the spilling. I'll change them all to silent spill/fill. Although, for now, perhaps we should just make all the non-JSArray cases slow paths. If I was going to remove runtime array from subclassing JSArray the fast path you proposed would not really work since we would still need to look for runtime arrays. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3316 >> +} > > I just looked at the code for "arrayConstructorPrivateFuncIsArrayConstructor" and I feel like we could do a lot better here. > That function just does a JSDyanmicCast on ArrayConstructor, but I don't think we have any inheritors of that > class. Why not just do a class info equality check here instead? I didn't bother making this fast because once we get to the DFG we nearly always prove that the node is a constant. We have 0 benchmarks that will call this code. I'll add a FIXME to make the change though. >> Source/JavaScriptCore/runtime/ArrayConstructor.h:100 >> + } > > Style: indentation. Fixed. >> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1029 >> + // We can > > Unfinished comment. Whoops, thanks. >> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1036 >> + return false; > > can this throw? Yes, if we run out of memory. >> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1080 >> + auto buffer = result->butterfly()->contiguousDouble().data(); > > I know we use auto elsewhere for similar operations, but I think it's easier to verify the memcpy is correct if you wrote out "double* buffer" here and below. That's fair I guess. I'll make the change.
Created attachment 274475 [details] Patch
Attachment 274475 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:295: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 274475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274475&action=review r=me with comment > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3296 > + silentFillAllRegisters(resultGPR); You're missing an exception check here after the silentFillAllRegisters call. This also means we probably don't have a test for this. I think it's worth adding one. And it's probably as easy as making the proxy revoked after it gets DFG compiled.
Created attachment 275128 [details] Patch for landing
Comment on attachment 275128 [details] Patch for landing Rejecting attachment 275128 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 275128, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: hunk FAILED -- saving rejects to file LayoutTests/js/Object-getOwnPropertyNames-expected.txt.rej patching file LayoutTests/js/dom/array-prototype-properties-expected.txt patching file LayoutTests/js/script-tests/Object-getOwnPropertyNames.js Hunk #1 FAILED at 70. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/js/script-tests/Object-getOwnPropertyNames.js.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/1065122
Created attachment 275131 [details] Patch for landing
Created attachment 275139 [details] Patch for landing
Comment on attachment 275139 [details] Patch for landing Clearing flags on attachment: 275139 Committed r198808: <http://trac.webkit.org/changeset/198808>
All reviewed patches have been landed. Closing bug.
The bots show a 3% regression on JSBench. This is likely the cause. Can you please have a look?
(In reply to comment #44) > The bots show a 3% regression on JSBench. > This is likely the cause. Can you please have a look? And it caused 16 JSC tests to fail on 32-bit testers: Bug 156016: REGRESSION (r198808): 16 32-bit JSC tests fail after adding support for ES6 Symbol.isConcatSpreadable <https://bugs.webkit.org/show_bug.cgi?id=156016>
I'm going to rollout the patch since there appear to be nontrivial issues to fix.
Rolled out in <http://trac.webkit.org/changeset/198844>
Reopen, because it was rolled out.
Committed r199128: <http://trac.webkit.org/changeset/199128>
Re-opened since this is blocked by bug 156348
Committed r199397: <http://trac.webkit.org/changeset/199397>
This change caused many crashes and other failures, rolling out. https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r199397%20(4385)/results.html
I think I have a fix running tests now.
Also this, I think: https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/2076/steps/webkit-32bit-jsc-test/logs/stdio
And the ARM bot too: https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Traditional%20Release/builds/241
just to document, fixed by https://trac.webkit.org/changeset/199402