Bug 141640 – Constructor returning null should construct an object instead of null

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Constructor returning null should construct an object instead of null

[Yusuke Suzuki]

function Test() {
  return null;
}

var result = new Test();  // result should be object, but now it's null.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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

[Geoffrey Garen]

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?

[Yusuke Suzuki]

(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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 246834 [details]
rev 4prototype

rev4 prototype. Applying FTL isString/isObject fix for supporting Symbol JSCell. Not applying merging is_function/is_object.

[Geoffrey Garen]

> 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.

[Geoffrey Garen]

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.

[Yusuke Suzuki]

(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 :)

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 246899 [details]
rev5 prototype

Add LLInt/DFG fix and optimizations. Add tests for regression. Not add tests for isString/isObject change

[Yusuke Suzuki]

(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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

Created attachment 246946 [details]
rev6 prototype

Fix SEGV bugs. Not add tests for isString/isObject change

[Yusuke Suzuki]

Created attachment 246962 [details]
rev7 prototype

Fix build failures. Not add tests for isString/isObject change

[Yusuke Suzuki]

Created attachment 247000 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Geoffrey Garen]

Comment on attachment 247000 [details]
Patch

r=me

[Filip Pizlo]

Comment on attachment 247000 [details]
Patch

LGTM too.

[Yusuke Suzuki]

Thank you for your reviews :)
So I'll fix ChangeLog typo and land it manually.

[Yusuke Suzuki]

Committed r180550: <http://trac.webkit.org/changeset/180550>

[Alexey Proskuryakov]

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?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 141957

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 247220 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Made sure that run-javascriptcore-tests passed.

[Filip Pizlo]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

OK, moved tests from layout-tests to stress tests and run-javascriptcore-tests passed.
I'll land it :)

[Yusuke Suzuki]

Committed r180587: <http://trac.webkit.org/changeset/180587>