Constructor returning null should construct an object instead of null
function Test() { return null; } var result = new Test(); // result should be object, but now it's null.
That is due to op_is_object returns true for null. This is because op_is_object is originally used for `typeof`, `typeof null` should return "object". I propose that adding new opcode op_is_object_or_null and use it for `typeof` operations.
Created attachment 246741 [details] rev1 prototype naive prototype. Now is_object becomes fairly simple. So we get a chance to optimize it in LLInt / DFG / FTL
Created attachment 246743 [details] rev2 prototype optimized prototype. but FTL isString/isObject predicate now has issue since I added Symbol JSCell type. I'll fix it in the next updated patch
Oops! This is my fault for removing op_ret_object_or_this. I believed that I could simplify things by removing an opcode. About your approach: If you did add an op_is_object_or_null, then you should remove the branch inserted by the BytecodeGenerator to check for is_function -- that is subsumed by op_is_object_or_null. But I wonder: Can we rescue the original idea of just reusing existing opcodes? It's a shame to have to add back an opcode for this oddball feature, which is rarely used. I believe an alternative way to fix this bug is just to add an is_null branch in the BytecodeGenerator. So, the logic would be: if (is_null) goto fail if (is_object) goto success if (is_function) goto success // fallthrough fail: return this success: return src What do you think?
(In reply to comment #5) > Oops! This is my fault for removing op_ret_object_or_this. I believed that I > could simplify things by removing an opcode. > > About your approach: If you did add an op_is_object_or_null, then you should > remove the branch inserted by the BytecodeGenerator to check for is_function > -- that is subsumed by op_is_object_or_null. Great! Thank you! I've missed it. > > But I wonder: Can we rescue the original idea of just reusing existing > opcodes? It's a shame to have to add back an opcode for this oddball > feature, which is rarely used. I believe that simplified op_is_object can be used in many areas. I'm now planning to use it in upgraded for-of implementation since it requires Type(value) == "Object" check. https://bugs.webkit.org/show_bug.cgi?id=141351 > > I believe an alternative way to fix this bug is just to add an is_null > branch in the BytecodeGenerator. So, the logic would be: > > if (is_null) > goto fail > if (is_object) > goto success > if (is_function) > goto success > // fallthrough > > fail: > return this > > success: > return src > > What do you think? I think using simplified op_is_object is preferrable than using the existing op_is_object since, 1. Exsiting op_is_object is a slow path opcode to handle masqueradesAsUndefined (typeof document.all). Introducing simplified is_object opcode that just checks value.isObject() enables JIT to avoid runtime function call and inline it. This improves performance. 2. Simplified op_is_object can be used to check the target is object or not. I believe that it can be more used in many places. However, introducing new bytecode back is not good and reducing bytecodes is greatly nice. So instead, I think op_is_object_or_null and op_is_function can be merged into one opcode since 1. They are both slow path opcodes. 2. They're only used for typeof (so it's rare opcodes) So calling a runtime function, bool operationIsObject(bool callable) { return callable ? is_function check : is_object_or_null check; } can merge them and it reduces one opcode. What do you think of?
Created attachment 246808 [details] rev3 prototype small bugfix. Not applying FTL isString/isObject fix yet. Not applying merging is_function/is_object_or_null yet.
Created attachment 246834 [details] rev 4prototype rev4 prototype. Applying FTL isString/isObject fix for supporting Symbol JSCell. Not applying merging is_function/is_object.
> I believe that simplified op_is_object can be used in many areas. > I'm now planning to use it in upgraded for-of implementation since it > requires Type(value) == "Object" check. > https://bugs.webkit.org/show_bug.cgi?id=141351 I see. If we need this opcode in more places for ES6, then I like it. It's a straight-forward opcode. I was only objecting to introducing an opcode solely for this constructor return feature. > However, introducing new bytecode back is not good and reducing bytecodes is > greatly nice. > So instead, I think op_is_object_or_null and op_is_function can be merged > into one opcode since > 1. They are both slow path opcodes. > 2. They're only used for typeof (so it's rare opcodes) > So calling a runtime function, > bool operationIsObject(bool callable) > { > return callable ? is_function check : is_object_or_null check; > } > can merge them and it reduces one opcode. > > What do you think of? Actually, I like this less since it's a little more opaque. Long-term, if we want to reduce the set of typeof optimization opcodes, I think the way to do it is to slightly improve the DFG's data flow analysis for typeof, and then remove the special typeof opcodes entirely. It should be straight-forward for the DFG to recognize "if (typeof x == 'object')" and so on, and just fold that into its existing type checking mechanism.
Comment on attachment 246834 [details] rev 4prototype This approach looks good to me. Two items of note: (1) Can you add a regression test to verify the constructor return case? Ideally, you should write a constructor that returns every type: boolean, null, string, etc. (2) I think the DFG abstract interpreter can do better. You should add an IsObject case, so the abstract interpreter will constant fold IsObject(object) into true, just like it can fold IsObjectOrNull(null) and so on.
(In reply to comment #9) > I see. If we need this opcode in more places for ES6, then I like it. It's a > straight-forward opcode. I was only objecting to introducing an opcode > solely for this constructor return feature. I see. > Actually, I like this less since it's a little more opaque. > > Long-term, if we want to reduce the set of typeof optimization opcodes, I > think the way to do it is to slightly improve the DFG's data flow analysis > for typeof, and then remove the special typeof opcodes entirely. It should > be straight-forward for the DFG to recognize "if (typeof x == 'object')" and > so on, and just fold that into its existing type checking mechanism. Make sense. Folding by DFG data flow analysis is preferable rather than specializing opcode in bytecompiler :)
(In reply to comment #10) > Comment on attachment 246834 [details] > rev 4prototype > > This approach looks good to me. Two items of note: Thank you for your comments. That's very helpful. > > (1) Can you add a regression test to verify the constructor return case? > Ideally, you should write a constructor that returns every type: boolean, > null, string, etc. Right. We need to add regression tests. After adding DFG/LLInt fix for Symbol support in rev5 prototype, I'll add tests and makes the complete patch with r? :) > > (2) I think the DFG abstract interpreter can do better. You should add an > IsObject case, so the abstract interpreter will constant fold > IsObject(object) into true, just like it can fold IsObjectOrNull(null) and > so on. Yes. DFG abstract interpreter support (& we can relax clobberize constraints for IsObject since it only touches typeinfo and it would be changed.) gives a chance to produce more efficient code. I'll add this.
Created attachment 246899 [details] rev5 prototype Add LLInt/DFG fix and optimizations. Add tests for regression. Not add tests for isString/isObject change
(In reply to comment #13) > Created attachment 246899 [details] > rev5 prototype > > Add LLInt/DFG fix and optimizations. Add tests for regression. Not add tests > for isString/isObject change Oops. SEGV occurs. I need to debug it.
Comment on attachment 246899 [details] rev5 prototype Attachment 246899 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6241625747488768 New failing tests: js/regress/string-cons-tower.html js/dom/dfg-make-rope-side-effects.html js/dom/line-column-numbers.html js/dom/toString-and-valueOf-override.html fast/dom/gc-acid3.html js/dfg-to-string-valueOf-becomes-bad.html js/dfg-to-string-toString-becomes-bad.html
Created attachment 246904 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 246899 [details] rev5 prototype Attachment 246899 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5804709461884928 New failing tests: js/regress/string-cons-tower.html js/dom/dfg-make-rope-side-effects.html js/dom/line-column-numbers.html js/dom/toString-and-valueOf-override.html fast/dom/gc-acid3.html js/dfg-to-string-valueOf-becomes-bad.html fast/workers/worker-copy-shared-blob-url.html js/dfg-to-string-toString-becomes-bad.html
Created attachment 246908 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 246946 [details] rev6 prototype Fix SEGV bugs. Not add tests for isString/isObject change
Created attachment 246962 [details] rev7 prototype Fix build failures. Not add tests for isString/isObject change
Created attachment 247000 [details] Patch
Comment on attachment 247000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247000&action=review Updated the patch. Ready for review. Added comments on the patch. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:-1943 > - Dropped. Thank you for your pointing! > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:826 > + break; If the constant value is set as a parameter of op_is_object, we examine it. > Source/JavaScriptCore/dfg/DFGClobberize.h:140 > + case IsObject: Since IsObject only read JSCell typeInfoType and it would be changed through structure transition, in clobberize path, IsObject claims that it doesn't perform read/write just like IsString. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1160 > + BadType, JSValueSource::unboxedCell(op1GPR), node->child1(), branchNotObject(op1GPR)); It's intended to check the incomming value is not object. (not checking string). So change it to branchNotObject. It returns false for String and *Symbol* JSCells. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3946 > + fastFalse.append(branchNotString(rightRegs.payloadGPR())); Here, checking notString (Since following pash is specialized for String). So use branchNotString. > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:645 > + // Jump to a slow case if both are strings or symbols (non object). Since the equality of symbols is not tested by pointer comparison, we need to exit to the slow path when the both values are String or Symbol.
Comment on attachment 247000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247000&action=review >> Source/JavaScriptCore/dfg/DFGClobberize.h:140 >> + case IsObject: > > Since IsObject only read JSCell typeInfoType and it would be changed through structure transition, > in clobberize path, IsObject claims that it doesn't perform read/write just like IsString. typo: would => would not I need to fix it in ChangeLog.
Comment on attachment 247000 [details] Patch r=me
Comment on attachment 247000 [details] Patch LGTM too.
Thank you for your reviews :) So I'll fix ChangeLog typo and land it manually.
Committed r180550: <http://trac.webkit.org/changeset/180550>
This change broke JSC tests: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20(Tests)/builds/3072/steps/jscore-test/logs/stdio https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20(Tests)/builds/3059/steps/jscore-test/logs/stdio Is anyone available to look into this now?
Re-opened since this is blocked by bug 141957
(In reply to comment #28) > This change broke JSC tests: > > https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20(Tests)/ > builds/3072/steps/jscore-test/logs/stdio > > https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20(Tests)/ > builds/3059/steps/jscore-test/logs/stdio > > Is anyone available to look into this now? Thank you. I'll look into it.
(In reply to comment #30) > (In reply to comment #28) > > This change broke JSC tests: > > > > https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20(Tests)/ > > builds/3072/steps/jscore-test/logs/stdio > > > > https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20(Tests)/ > > builds/3059/steps/jscore-test/logs/stdio > > > > Is anyone available to look into this now? > > Thank you. I'll look into it. Ah, simply, specifying `noInline` to LayoutTests's doToPrimitive function solves this problem. I'll upload the minor fixed patch.
Created attachment 247220 [details] Patch
Comment on attachment 247220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247220&action=review Added comment about changes from the previous patch. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1185 > + if (!(forNode(node->child1()).m_type & ~(SpecFullNumber | SpecBoolean | SpecString | SpecCellOther))) { One large change from the previous change is here. In ToPrimitive operation, Symbol is also passed through. So the speculated type should consider symbols. Currently, Symbol is classified into SpecCellOther. So I've specified it. In the future, we need to introduce SpecSymbol and optimization for Symbols. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1193 > + forNode(node).setType((SpecHeapTop & ~SpecCell) | SpecString | SpecCellOther); Ditto. > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:372 > + if (m_state.forNode(node->child1()).m_type & ~(SpecFullNumber | SpecBoolean | SpecString | SpecCellOther)) Ditto. > LayoutTests/js/script-tests/dfg-to-primitive-pass-symbol.js:19 > +noInline(doToPrimitive); Add noInline. It avoids the current issue.
Made sure that run-javascriptcore-tests passed.
Comment on attachment 247220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247220&action=review > LayoutTests/js/script-tests/dfg-to-primitive-pass-symbol.js:23 > +while (!dfgCompiled({f:doToPrimitive})) { > + doToPrimitive(); > +} There are a number of things about your tests that are undesirable. First, you're using the old dfgCompiled mechanism. We don't use this mechanism anymore for new tests. Instead, we rely on the fact that 10,000 function invocations of a noInline function is guaranteed to be enough to get that function compiled with the highest tier JIT when you're testing with run-javascriptcore-tests. This is because run-javascriptcore-tests (via run-jsc-stress-tests, which does the real work) runs each test in a "no-cjit" mode that also forces all thresholds to be deterministic enough to ensure that 10,000 invocations get you to the FTL. Second, you're writing these as layout tests. That's rather cumbersome. We tend to write new tests, especially compiler tests like these, as "stress" tests in Source/JavaScriptCore/tests/stress. I think that with these changes, your tests are fine. But the timeout that you got and the subsequent rollout, plus the fact that you have to write boilerplate like the .html file, the -expected.txt file, the descrition() line, etc are all examples of why layout tests are not a great way of writing new JavaScriptCore tests.
Comment on attachment 247220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247220&action=review >> LayoutTests/js/script-tests/dfg-to-primitive-pass-symbol.js:23 >> +} > > There are a number of things about your tests that are undesirable. > > First, you're using the old dfgCompiled mechanism. We don't use this mechanism anymore for new tests. Instead, we rely on the fact that 10,000 function invocations of a noInline function is guaranteed to be enough to get that function compiled with the highest tier JIT when you're testing with run-javascriptcore-tests. This is because run-javascriptcore-tests (via run-jsc-stress-tests, which does the real work) runs each test in a "no-cjit" mode that also forces all thresholds to be deterministic enough to ensure that 10,000 invocations get you to the FTL. > > Second, you're writing these as layout tests. That's rather cumbersome. We tend to write new tests, especially compiler tests like these, as "stress" tests in Source/JavaScriptCore/tests/stress. > > I think that with these changes, your tests are fine. But the timeout that you got and the subsequent rollout, plus the fact that you have to write boilerplate like the .html file, the -expected.txt file, the descrition() line, etc are all examples of why layout tests are not a great way of writing new JavaScriptCore tests. Thank you for your review and very helpful pointing! Source/JavaScriptCore/tests/stress looks super awesome. I'll change my test 1. Move & Modify this layout-tests into stress tests (except for js/dom/constructor-with-return-masquerades.html, because it depends on `document.all`) 2. use 10000 loop instead of using dfgCompiled.
OK, moved tests from layout-tests to stress tests and run-javascriptcore-tests passed. I'll land it :)
Committed r180587: <http://trac.webkit.org/changeset/180587>