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.
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
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
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
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
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.
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.
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.
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.
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
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
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
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
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.
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.
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
(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>
2016-03-17 11:50 PDT, Keith Miller
2016-03-17 11:58 PDT, Keith Miller
2016-03-17 12:09 PDT, Keith Miller
2016-03-17 12:56 PDT, Build Bot
2016-03-17 12:59 PDT, Build Bot
2016-03-17 13:01 PDT, Build Bot
2016-03-17 13:12 PDT, Build Bot
2016-03-17 17:02 PDT, Keith Miller
2016-03-17 17:46 PDT, Keith Miller
2016-03-17 18:11 PDT, Keith Miller
2016-03-17 19:11 PDT, Build Bot
2016-03-17 19:15 PDT, Build Bot
2016-03-17 19:24 PDT, Build Bot
2016-03-17 19:28 PDT, Build Bot
2016-03-18 16:30 PDT, Keith Miller
2016-03-29 14:09 PDT, Keith Miller
2016-03-29 14:20 PDT, Keith Miller
2016-03-29 15:01 PDT, Keith Miller